Don't add extra 8 byte to DOM wrappers in non-oilpan builds

Don't add extra 8 byte to DOM wrappers in non-oilpan builds

Imagine a DOM object that is RefCountedWillBeGarbageCollected. In oilpan builds, we need extra 8 byte on its DOM wrapper to store a persistent handle to the DOM object. On the other hand, in non-oilpan builds, we don't need the 8 byte. This CL removes the 8 byte from the DOM wrapper.

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

[haraken, 2014-06-03 08:42:58]

PTAL

[zerny-chromium, 2014-06-03 08:49:03]

https://codereview.chromium.org/314603003/diff/1/Source/bindings/templates/in...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/314603003/diff/1/Source/bindings/templates/in...
Source/bindings/templates/interface.h:121: static const int internalFieldCount =
v8DefaultWrapperInternalFieldCount + {{custom_internal_field_counter}};
I find the side-effect above confusing, could we not just set this
internalFieldCount with a + 1, as done in the case below. If not, isn't the case
below missing a "set x = x + 1"?

[haraken, 2014-06-03 08:55:40]

https://codereview.chromium.org/314603003/diff/1/Source/bindings/templates/in...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/314603003/diff/1/Source/bindings/templates/in...
Source/bindings/templates/interface.h:121: static const int internalFieldCount =
v8DefaultWrapperInternalFieldCount + {{custom_internal_field_counter}};
On 2014/06/03 08:49:03, zerny-chromium wrote:
> I find the side-effect above confusing, could we not just set this
> internalFieldCount with a + 1, as done in the case below. If not, isn't the
case
> below missing a "set x = x + 1"?

That looks nicer. Done.

[zerny-chromium, 2014-06-03 08:59:42]

On 2014/06/03 08:55:40, haraken wrote:
>
https://codereview.chromium.org/314603003/diff/1/Source/bindings/templates/in...
> File Source/bindings/templates/interface.h (right):
> 
>
https://codereview.chromium.org/314603003/diff/1/Source/bindings/templates/in...
> Source/bindings/templates/interface.h:121: static const int internalFieldCount
=
> v8DefaultWrapperInternalFieldCount + {{custom_internal_field_counter}};
> On 2014/06/03 08:49:03, zerny-chromium wrote:
> > I find the side-effect above confusing, could we not just set this
> > internalFieldCount with a + 1, as done in the case below. If not, isn't the
> case
> > below missing a "set x = x + 1"?
> 
> That looks nicer. Done.

And we are sure that nothing after that uses custom_internal_field_counter and
expects it to be equal to the last offset?
I can't really see where that piece of state is stored/used.

[Mads Ager (chromium), 2014-06-03 09:10:48]

DBC: This removes the overhead for non-oilpan builds. Do we have a tracking bug
for attempting to reduce the memory overhead for oilpan builds as well (for
instance by just storing the Persistent instead of the raw pointer)? That should
be a separate change. It is not clear to me how much it would affect performance
to have to extract the native pointer from the Persistent* instead of having it
directly in the wrapper. We need to measure that carefully as a separate change.

[Mads Ager (chromium), 2014-06-03 09:16:27]

LGTM

The generated code now has lines with "+ 1 + 1" instead of "+ 2". Is that
related to the 'set' operation that has been removed?

[zerny-chromium, 2014-06-03 09:20:19]

https://codereview.chromium.org/314603003/diff/20001/Source/bindings/template...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/314603003/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.h:107: {% set custom_internal_field_counter
= 0 %}
This looks like the declaration of the field counter and its only uses are
below, so it looks like local state :-) So lgtm with either the explicit + 1 or
with the increments so long as we use the same in both cases. Whichever you
prefer.

[haraken, 2014-06-03 09:29:55]

> DBC: This removes the overhead for non-oilpan builds. Do we have a tracking
bug
> for attempting to reduce the memory overhead for oilpan builds as well (for
> instance by just storing the Persistent instead of the raw pointer)? That
should
> be a separate change. It is not clear to me how much it would affect
performance
> to have to extract the native pointer from the Persistent* instead of having
it
> directly in the wrapper. We need to measure that carefully as a separate
change.

No tracking bug yet. I agree that we should investigate the performance & memory
impact of wrapper layouts.

(FWIW, I guess that adding extra 8 byte won't affect real-world memory usage but
adding the overhead of extracting a C++ pointer from a persistent handle will
regress performance of Dromaeo. I am planning to measure the memory impact of
adding extra 8 or 16 byte to Node or CSSValue in order to measure the memory
impact of making all DOM objects ScriptWrappable and add virtual methods to
ScriptWrappable.)

https://codereview.chromium.org/314603003/diff/20001/Source/bindings/template...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/314603003/diff/20001/Source/bindings/template...
Source/bindings/templates/interface.h:107: {% set custom_internal_field_counter
= 0 %}
On 2014/06/03 09:20:19, zerny-chromium wrote:
> This looks like the declaration of the field counter and its only uses are
> below, so it looks like local state :-) So lgtm with either the explicit + 1
or
> with the increments so long as we use the same in both cases. Whichever you
> prefer.

Fixed the "+ 1 + 1" to "+ 2".

[haraken, 2014-06-03 09:29:59]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-03 09:31:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/314603003/10008

[zerny-chromium, 2014-06-03 09:35:23]

https://codereview.chromium.org/314603003/diff/10008/Source/bindings/template...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/314603003/diff/10008/Source/bindings/template...
Source/bindings/templates/interface.h:125: {% set custom_internal_field_counter
= custom_internal_field_counter + 1 %}
It looks like this does not work because of phase differences. The meta-level
code does not care about the #if so it increments the count regardless. That
gives an incorrect offset in the output :-/

[haraken, 2014-06-03 09:47:50]

The CQ bit was unchecked by haraken@chromium.org

[haraken, 2014-06-03 10:41:14]

On 2014/06/03 09:35:23, zerny-chromium wrote:
>
https://codereview.chromium.org/314603003/diff/10008/Source/bindings/template...
> File Source/bindings/templates/interface.h (right):
> 
>
https://codereview.chromium.org/314603003/diff/10008/Source/bindings/template...
> Source/bindings/templates/interface.h:125: {% set
custom_internal_field_counter
> = custom_internal_field_counter + 1 %}
> It looks like this does not work because of phase differences. The meta-level
> code does not care about the #if so it increments the count regardless. That
> gives an incorrect offset in the output :-/

Good point... Reverted back to the "+ 1 + 1" approach. Code beauty of the IDL
compiler would be important than that of generated code.

[haraken, 2014-06-03 10:41:18]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-03 10:42:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/314603003/40001

[commit-bot: I haz the power, 2014-06-03 11:55:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6777)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10337)

[commit-bot: I haz the power, 2014-06-03 12:50:51]

Change committed as 175361