IDL: Remove a FIXME comment for using local variables

IDL: Remove a FIXME comment for using local variables

Using local variable for isolate in attribute_getter will add
complexity and won't pay for it.

BUG=345503
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177077

[bashi, 2014-06-27 06:20:31]

Could you take a look?

https://codereview.chromium.org/359703002/diff/1/Source/bindings/templates/at...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/359703002/diff/1/Source/bindings/templates/at...
Source/bindings/templates/attributes.cpp:51: {# FIXME: consider always using a
local variable for value #}
BTW, what's your take on this comment? There is a comment on getter_context() in
v8_attribute.py for this.

    # Normally we can inline the function call into the return statement to
    # avoid the overhead of using a Ref<> temporary, but for some cases
    # (nullable types, EventHandler, [CachedAttribute], or if there are
    # exceptions), we need to use a local variable.
    # FIXME: check if compilers are smart enough to inline this, and if so,
    # always use a local variable (for readability and CG simplicity).

The behavior of |cpp_value_original| and |cpp_value| is a bit confusing. If we
can always use local vars for |cpp_value|, I think we can remove
|cpp_value_original|. How I can measure the performance impact of this?

[Jens Widell, 2014-06-27 06:31:23]

https://codereview.chromium.org/359703002/diff/1/Source/bindings/templates/at...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/359703002/diff/1/Source/bindings/templates/at...
Source/bindings/templates/attributes.cpp:51: {# FIXME: consider always using a
local variable for value #}
On 2014/06/27 06:20:31, bashi1 wrote:
> The behavior of |cpp_value_original| and |cpp_value| is a bit confusing. If we
> can always use local vars for |cpp_value|, I think we can remove
> |cpp_value_original|. How I can measure the performance impact of this?

If we keep the use of a local optional, then we could at least change the
condition below to

  {% if attribute.cpp_value_original %}

to avoid duplicating the actual condition, which must be the same here and in
getter_context() anyway.

[haraken, 2014-06-27 06:35:10]

LGTM

https://codereview.chromium.org/359703002/diff/1/Source/bindings/templates/at...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/359703002/diff/1/Source/bindings/templates/at...
Source/bindings/templates/attributes.cpp:51: {# FIXME: consider always using a
local variable for value #}
On 2014/06/27 06:20:31, bashi1 wrote:
> BTW, what's your take on this comment? There is a comment on getter_context()
in
> v8_attribute.py for this.
> 
>     # Normally we can inline the function call into the return statement to
>     # avoid the overhead of using a Ref<> temporary, but for some cases
>     # (nullable types, EventHandler, [CachedAttribute], or if there are
>     # exceptions), we need to use a local variable.
>     # FIXME: check if compilers are smart enough to inline this, and if so,
>     # always use a local variable (for readability and CG simplicity).
> 
> The behavior of |cpp_value_original| and |cpp_value| is a bit confusing. If we
> can always use local vars for |cpp_value|, I think we can remove
> |cpp_value_original|. How I can measure the performance impact of this?

You can run Dromaeo to see performance (http://dromaeo.com/?dom). If it looks
OK, we can just land the CL and watch perf bots.

I agree that we should remove the confusion about |cpp_value_original| and
|cpp_value|.

[bashi, 2014-06-27 06:42:26]

On 2014/06/27 06:35:10, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/359703002/diff/1/Source/bindings/templates/at...
> File Source/bindings/templates/attributes.cpp (right):
> 
>
https://codereview.chromium.org/359703002/diff/1/Source/bindings/templates/at...
> Source/bindings/templates/attributes.cpp:51: {# FIXME: consider always using a
> local variable for value #}
> On 2014/06/27 06:20:31, bashi1 wrote:
> > BTW, what's your take on this comment? There is a comment on
getter_context()
> in
> > v8_attribute.py for this.
> > 
> >     # Normally we can inline the function call into the return statement to
> >     # avoid the overhead of using a Ref<> temporary, but for some cases
> >     # (nullable types, EventHandler, [CachedAttribute], or if there are
> >     # exceptions), we need to use a local variable.
> >     # FIXME: check if compilers are smart enough to inline this, and if so,
> >     # always use a local variable (for readability and CG simplicity).
> > 
> > The behavior of |cpp_value_original| and |cpp_value| is a bit confusing. If
we
> > can always use local vars for |cpp_value|, I think we can remove
> > |cpp_value_original|. How I can measure the performance impact of this?
> 
> You can run Dromaeo to see performance (http://dromaeo.com/?dom). If it looks
> OK, we can just land the CL and watch perf bots.
> 
> I agree that we should remove the confusion about |cpp_value_original| and
> |cpp_value|.

Thanks for your inputs Jens and haraken! I'll check the performance impact and
then try to make a CL to remove the duplication based on the result.

[bashi, 2014-06-27 06:42:49]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-06-27 06:43:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bashi@chromium.org/359703002/1

[commit-bot: I haz the power, 2014-06-27 07:38:23]

Change committed as 177077