Move privateTemplateUniqueKey / sharedTemplateUniqueKey to .bss section

Move privateTemplateUniqueKey / sharedTemplateUniqueKey in the 
generated bindings to .bss section instead of .data by
getting rid of the pointer and using a simple static 
uninitialized int.

This decreases our binary size. This also reduces the number of
relocations in libchromeview_prebuilt.so by 18 according to
relinfo.

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

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

[Inactive, 2013-10-18 19:08:57]

Relinfo output:
- Before: out/Release/lib/libchromeview_prebuilt.so: 240776 relocations, 240715
relative (99%), 329 PLT entries, 0 for local syms (0%), 0 users
- After: out/Release/lib/libchromeview_prebuilt.so: 240766 relocations, 240705
relative (99%), 329 PLT entries, 0 for local syms (0%), 0 users

$ objdump -x out/Release/lib/libchromeview_prebuilt.so | grep
privateTemplateUniqueKey
01fca40c l     O .rodata	00000004
_ZZN7WebCore18LocationV8InternalL16assignAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
01fca410 l     O .rodata	00000004
_ZZN7WebCore18LocationV8InternalL17replaceAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
01fca6e4 l     O .rodata	00000004
_ZZN7WebCore29V8HTMLAudioElementConstructor11GetTemplateEPN2v87IsolateENS_16WrapperWorldTypeEE24privateTemplateUniqueKey
01fca938 l     O .rodata	00000004
_ZZN7WebCore19DOMWindowV8InternalL15closeAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
01fca93c l     O .rodata	00000004
_ZZN7WebCore19DOMWindowV8InternalL14blurAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
01fca940 l     O .rodata	00000004
_ZZN7WebCore19DOMWindowV8InternalL18toStringAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
01fca944 l     O .rodata	00000004
_ZZN7WebCore19DOMWindowV8InternalL21postMessageAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
01fca948 l     O .rodata	00000004
_ZZN7WebCore19DOMWindowV8InternalL15focusAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
01fca9e8 l     O .rodata	00000004
_ZZN7WebCore30V8HTMLOptionElementConstructor11GetTemplateEPN2v87IsolateENS_16WrapperWorldTypeEE24privateTemplateUniqueKey
01fce238 l     O .rodata	00000004
_ZZN7WebCore29V8HTMLImageElementConstructor11GetTemplateEPN2v87IsolateENS_16WrapperWorldTypeEE24privateTemplateUniqueKey
01fce23c l     O .rodata	00000004
_ZZN7WebCore12_GLOBAL__N_130primitiveWrapperObjectTemplateEPN2v87IsolateEE24privateTemplateUniqueKey
01fce240 l     O .rodata	00000004
_ZZN7WebCore12_GLOBAL__N_137promiseEveryEnvironmentObjectTemplateEPN2v87IsolateEE24privateTemplateUniqueKey
01fce244 l     O .rodata	00000004
_ZZN7WebCore12_GLOBAL__N_140wrapperCallbackEnvironmentObjectTemplateEPN2v87IsolateEE24privateTemplateUniqueKey
01fce248 l     O .rodata	00000004
_ZZN7WebCore12_GLOBAL__N_122internalObjectTemplateEPN2v87IsolateEE24privateTemplateUniqueKey

$ objdump -x out/Release/lib/libchromeview_prebuilt.so.old | grep
privateTemplateUniqueKey
022ebfb8 l     O .data	00000004
_ZZN7WebCore18LocationV8InternalL17replaceAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
022ec304 l     O .data	00000004
_ZZN7WebCore18LocationV8InternalL16assignAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
022ed1c8 l     O .data	00000004
_ZZN7WebCore29V8HTMLAudioElementConstructor11GetTemplateEPN2v87IsolateENS_16WrapperWorldTypeEE24privateTemplateUniqueKey
022ee050 l     O .data	00000004
_ZZN7WebCore19DOMWindowV8InternalL18toStringAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
022ee114 l     O .data	00000004
_ZZN7WebCore19DOMWindowV8InternalL15focusAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
022ee284 l     O .data	00000004
_ZZN7WebCore19DOMWindowV8InternalL15closeAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
022ee2ac l     O .data	00000004
_ZZN7WebCore19DOMWindowV8InternalL14blurAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
022ee2f0 l     O .data	00000004
_ZZN7WebCore19DOMWindowV8InternalL21postMessageAttrGetterEN2v85LocalINS1_6StringEEERKNS1_20PropertyCallbackInfoINS1_5ValueEEEE24privateTemplateUniqueKey
022ee494 l     O .data	00000004
_ZZN7WebCore30V8HTMLOptionElementConstructor11GetTemplateEPN2v87IsolateENS_16WrapperWorldTypeEE24privateTemplateUniqueKey
022ef7d0 l     O .data	00000004
_ZZN7WebCore29V8HTMLImageElementConstructor11GetTemplateEPN2v87IsolateENS_16WrapperWorldTypeEE24privateTemplateUniqueKey
02326d88 l     O .bss	00000004
_ZZN7WebCore12_GLOBAL__N_130primitiveWrapperObjectTemplateEPN2v87IsolateEE24privateTemplateUniqueKey
02326d8c l     O .bss	00000004
_ZZN7WebCore12_GLOBAL__N_140wrapperCallbackEnvironmentObjectTemplateEPN2v87IsolateEE24privateTemplateUniqueKey
02326d90 l     O .bss	00000004
_ZZN7WebCore12_GLOBAL__N_137promiseEveryEnvironmentObjectTemplateEPN2v87IsolateEE24privateTemplateUniqueKey
02326d94 l     O .bss	00000004
_ZZN7WebCore12_GLOBAL__N_122internalObjectTemplateEPN2v87IsolateEE24privateTemplateUniqueKey

[Inactive, 2013-10-18 19:28:04]

Relinfo now gives:
out/Release/lib/libchromeview_prebuilt.so: 240758 relocations, 240697 relative
(99%), 329 PLT entries, 0 for local syms (0%), 0 users

(So 18 relocations less)

After doing the same thing for sharedTemplateUniqueKey as well.

[abarth-chromium, 2013-10-18 20:00:56]

lgtm

[eseidel, 2013-10-18 20:02:22]

I *love* that you're doing this.  You should teach us all how at the next
BlinkOn. :)

10 out of 200k sounds like we have a while to go before this is noticable
however.

[commit-bot: I haz the power, 2013-10-18 20:05:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/29553002/50001

[commit-bot: I haz the power, 2013-10-19 02:37:14]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-19 02:40:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/29553002/50001

[commit-bot: I haz the power, 2013-10-19 03:32:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[digit1, 2013-10-19 08:04:16]

https://codereview.chromium.org/29553002/diff/50001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/29553002/diff/50001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:1189: static const int
privateTemplateUniqueKey = 0;
This looks really good.

For the record, you can also avoid allocating a new global constant variable
entirely if you use the current function's address as the unique key (instead of
the address of an unused constant variable). This also makes the code slightly
smaller because compute the function address is easy in a PIC shared library,
while accessing the variable's address typically requires a GOT indirection and
one more memory load.

There are apparently only six variables affected by this patch, so that might
not be high priority, but consider this for future generator changes :)

[commit-bot: I haz the power, 2013-10-19 13:17:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/29553002/50001

[commit-bot: I haz the power, 2013-10-19 15:41:14]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-20 20:49:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/29553002/50001

[commit-bot: I haz the power, 2013-10-20 23:14:24]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[marja, 2013-10-21 07:26:05]

Cool, thanks for doing this!

[commit-bot: I haz the power, 2013-10-21 13:21:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/29553002/50001

[commit-bot: I haz the power, 2013-10-21 13:22:19]

Change committed as 160087

[alecflett, 2013-10-21 22:44:23]

On 2013/10/21 13:22:19, I haz the power (commit-bot) wrote:
> Change committed as 160087

This was reverted because it was giving some pretty strange behavior on the
windows trybots.

Note above that win_blink_rel failed quite a bit before the CQ decided to land
this anyway.

[Inactive, 2013-10-22 13:07:43]

On 2013/10/21 22:44:23, alecflett wrote:
> On 2013/10/21 13:22:19, I haz the power (commit-bot) wrote:
> > Change committed as 160087
> 
> This was reverted because it was giving some pretty strange behavior on the
> windows trybots.
> 
> Note above that win_blink_rel failed quite a bit before the CQ decided to land
> this anyway.

My bad, I thought the windows bot trouble was unrelated as the bots have not
been very reliable lately. I forced the commit since all other bots were fine,
sorry. I am reopening this and will investigate why it fails on Windows.

Thanks for reverting Alec.

[Inactive, 2013-10-22 14:25:42]

https://codereview.chromium.org/29553002/diff/50001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/29553002/diff/50001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:1189: static const int
privateTemplateUniqueKey = 0;
On 2013/10/19 08:04:17, digit1 wrote:
> This looks really good.
> 
> For the record, you can also avoid allocating a new global constant variable
> entirely if you use the current function's address as the unique key (instead
of
> the address of an unused constant variable). This also makes the code slightly
> smaller because compute the function address is easy in a PIC shared library,
> while accessing the variable's address typically requires a GOT indirection
and
> one more memory load.
> 
> There are apparently only six variables affected by this patch, so that might
> not be high priority, but consider this for future generator changes :) 

So it looks like msvc optimizes out the static variable if I mark it as const.
Thus the failures. One solution would be to not mark it as const. This would
still get rid of the relocations but it would not move them to .rodata sections
:/

Taking the address of the current static function sounds interesting. One issue
is that we sometimes need 2 identifiers in the same function:
privateTemplateUniqueKey and sharedTemplateUniqueKey (see below). Those need to
be different.

[marja, 2013-10-22 16:13:38]

Ha, I was this written but unposted:

I'm paranoidically worried that some compiler might not do the right thing.
Like, somehow think it's ok to clamp them together or not reserve memory to them
or... (After all, even if we can get the address of the static var and give it
forward, whoever is given it cannot change it since it's a const int, so maybe
some compilers reason it's okay to not reserve memory for each one
individually.)

But I couldn't find the relevant ABI reference to say that it's guaranteed that
the right thing happens (or the other way around).

-- so apparently it's not so paranoid.

[abarth-chromium, 2013-10-22 18:49:07]

Wouldn't taking the address of the function cause problem with identical code
folding?  Two functions might end up with the same address if their bodies are
identical.

[Inactive, 2013-10-25 14:39:54]

I removed the 'const' to make msvc happy and updated the description
accordingly. Can we land it this way or does someone as a better proposal?

[abarth-chromium, 2013-10-25 16:08:00]

LGTM

I don't have a better proposal.  Maybe digit has a clever idea.

[digit1, 2013-10-25 16:28:31]

LGTM

Adam, it's true that taking the function's address is dangerous, the compiler
does indeed coalesces identical functions to the same address in the final
binary. I forgot about this :-)

I don't have an idea for something better. Unless we come up with a global and
shared list of constants, but this may be impractical depending on how these
files are generated, so let's accept this patch as is for now :-)

[commit-bot: I haz the power, 2013-10-25 17:33:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/29553002/460001

[commit-bot: I haz the power, 2013-10-25 17:33:44]

Failed to apply patch for
Source/bindings/tests/results/V8TestActiveDOMObject.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/tests/results/V8TestActiveDOMObject.cpp
  Hunk #1 succeeded at 136 with fuzz 1.
  Hunk #2 FAILED at 150.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/bindings/tests/results/V8TestActiveDOMObject.cpp.rej

Patch:       Source/bindings/tests/results/V8TestActiveDOMObject.cpp
Index: Source/bindings/tests/results/V8TestActiveDOMObject.cpp
diff --git a/Source/bindings/tests/results/V8TestActiveDOMObject.cpp
b/Source/bindings/tests/results/V8TestActiveDOMObject.cpp
index
8c8a270e26eeb9ecfd2ddb289140397600583aa6..4a64e843729f464cdca5a0f46f74d2288f815e82
100644
--- a/Source/bindings/tests/results/V8TestActiveDOMObject.cpp
+++ b/Source/bindings/tests/results/V8TestActiveDOMObject.cpp
@@ -136,7 +136,7 @@ static void postMessageMethodCallback(const
v8::FunctionCallbackInfo<v8::Value>&
 static void postMessageAttributeGetter(v8::Local<v8::String> name, const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
     // This is only for getting a unique pointer which we can pass to
privateTemplate.
-    static const char* privateTemplateUniqueKey = "postMessagePrivateTemplate";
+    static int privateTemplateUniqueKey;
     WrapperWorldType currentWorldType = worldType(info.GetIsolate());
     V8PerIsolateData* data = V8PerIsolateData::from(info.GetIsolate());
     v8::Handle<v8::FunctionTemplate> privateTemplate =
data->privateTemplate(currentWorldType, &privateTemplateUniqueKey,
TestActiveDOMObjectV8Internal::postMessageMethodCallback, v8Undefined(),
v8::Signature::New(V8PerIsolateData::from(info.GetIsolate())->rawTemplate(&V8TestActiveDOMObject::info,
currentWorldType)), 1);
@@ -150,7 +150,7 @@ static void postMessageAttributeGetter(v8::Local<v8::String>
name, const v8::Pro
     }
     TestActiveDOMObject* imp = V8TestActiveDOMObject::toNative(holder);
     if (!BindingSecurity::shouldAllowAccessToFrame(imp->frame(),
DoNotReportSecurityError)) {
-        static const char* sharedTemplateUniqueKey =
"postMessageSharedTemplate";
+        static int sharedTemplateUniqueKey;
         v8::Handle<v8::FunctionTemplate> sharedTemplate =
data->privateTemplate(currentWorldType, &sharedTemplateUniqueKey,
TestActiveDOMObjectV8Internal::postMessageMethodCallback, v8Undefined(),
v8::Signature::New(V8PerIsolateData::from(info.GetIsolate())->rawTemplate(&V8TestActiveDOMObject::info,
currentWorldType)), 1);
         v8SetReturnValue(info, sharedTemplate->GetFunction());
         return;

[commit-bot: I haz the power, 2013-10-25 18:24:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/29553002/570001

[commit-bot: I haz the power, 2013-10-25 19:16:58]

Change committed as 160609