MakeNames: Generate global constants in a loop rather than in sequence.

Generate global constants in a loop rather than in sequence.

The generated code just repeated the same code over and over again
which resulted in larger machine code than necessary. Doing the
same thing with a data table and a loop makes the code
more compact and efficient.

clang x64 diff:
Total change: -72804 bytes
==========================
  281 added, totalling +21557 bytes across 1 sources
  1482 removed, totalling -10809 bytes across 1 sources
  2 grown, for a net change of +2 bytes (3 bytes before, 5 bytes after) across 1 sources
  19 shrunk, for a net change of -83554 bytes (87001 bytes before, 3447 bytes after) across 17 sources

BUG=

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

[Daniel Bratell, 2014-09-25 16:27:10]

Patchset #1 (id:1) has been deleted

[Daniel Bratell, 2014-09-25 16:28:08]

Patchset #1 (id:20001) has been deleted

[Daniel Bratell, 2014-09-25 16:37:12]

bratell@opera.com changed reviewers:
+ haraken@chromium.org, jl@opera.com

[Daniel Bratell, 2014-09-25 16:37:12]

Please take a look at this patch that changes code from just repeating itself to
looping. It cuts away quite a bit of machine code (70 KB).

[Jens Widell, 2014-09-25 17:16:17]

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl (right):

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl:23: const
{{namespace}}QualifiedName& {{tag|symbol}}Tag =
reinterpret_cast<{{namespace}}QualifiedName*>(&{{suffix}}TagStorage)[{{loop.index0}}];
Could you declare an

  const {{namespace}}QualifiedName* const {{namespace}}Tags =
reinterpret_cast<{{namespace}}QualifiedName*>({{namespace}}TagStorage);

and use throughout, to simplify?

[Daniel Bratell, 2014-09-25 17:48:42]

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl (right):

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl:23: const
{{namespace}}QualifiedName& {{tag|symbol}}Tag =
reinterpret_cast<{{namespace}}QualifiedName*>(&{{suffix}}TagStorage)[{{loop.index0}}];
On 2014/09/25 17:16:16, Jens Widell wrote:
> Could you declare an
> 
>   const {{namespace}}QualifiedName* const {{namespace}}Tags =
> reinterpret_cast<{{namespace}}QualifiedName*>({{namespace}}TagStorage);
> 
> and use throughout, to simplify?

I've tried it but it generates warnings like this:

gen/blink/core/HTMLNames.cpp:33:26: warning: declaration requires a global
constructor [-Wglobal-constructors]
const HTMLQualifiedName& baseTag = Tags[10];


(Actually I had one const less so I tried again with exactly what you wrote
(with one {{namespace}} changed to {{suffix}}) and it still doesn't work.

But it doesn't matter. It collapses to no machine code and it adds one less
symbol to the scope so I can easily pretend I like it this way. :-)

[Jens Widell, 2014-09-25 18:02:47]

On 2014/09/25 17:48:42, Daniel Bratell wrote:
> But it doesn't matter. It collapses to no machine code and it adds one less
> symbol to the scope so I can easily pretend I like it this way. :-)

Well, occasionally someone needs to read the code generator and understand it,
and it's probably not more understandable than the code it generates.

IOW, I disagree with "does not matter". "Does not work" is a better argument
against my suggestion. :-)

[Daniel Bratell, 2014-09-25 19:40:49]

On 2014/09/25 18:02:47, Jens Widell wrote:
> On 2014/09/25 17:48:42, Daniel Bratell wrote:
> > But it doesn't matter. It collapses to no machine code and it adds one less
> > symbol to the scope so I can easily pretend I like it this way. :-)
> 
> Well, occasionally someone needs to read the code generator and understand it,
> and it's probably not more understandable than the code it generates.
> 
> IOW, I disagree with "does not matter". "Does not work" is a better argument
> against my suggestion. :-)

Well, I just meant that it didn't, in my mind, change the code from acceptable
to unacceptable or the other way around. In that way it didn't matter which form
we were forced to use.

[Jens Widell, 2014-09-26 06:09:46]

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/MakeNames.cpp.tmpl (right):

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/MakeNames.cpp.tmpl:19: {% for entry in
entries|sort(case_sensitive=True) %}
Since we're sorting a list of structures (not strings) Jinja won't do
case-insensitive sorting either way, unless you also use attribute='name'. If
clarity is what's desired, adding attribute= is perhaps what we should do here.
Having just case_sensitive= is misleading.

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/MakeNames.cpp.tmpl:35: { "{{entry|cpp_name}}",
{{entry|cpp_name|hash}}, {{entry|cpp_name|length}} },
I don't really understand why "|cpp_name" is used here instead of ".name".

The "|cpp_name" filter is the same as the "|script_name" filter unless there's
an ImplementedAs= override. The "|script_name" filter returns
os.path.basename(entry['name']). This may make sense when the name is the name
of an interface or something like that, but for tag names, it seems irrelevant.
And *if* there was a tag with an ImplementedAs= for some reason, surely the real
name would be what should be used in this string literal in either case?

(Both implemented in Source/build/scripts/name_utilities.py.)

[Daniel Bratell, 2014-09-26 07:59:29]

I've changed the sorting. Since it sorts something that isn't directly visible
in the output data the results might still not be obvious to a human, but at
least it is now well defined exactly what we sort (the name, path and all)
instead of sorting dict objects.

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/MakeNames.cpp.tmpl (right):

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/MakeNames.cpp.tmpl:19: {% for entry in
entries|sort(case_sensitive=True) %}
On 2014/09/26 06:09:46, Jens Widell wrote:
> Since we're sorting a list of structures (not strings) Jinja won't do
> case-insensitive sorting either way, unless you also use attribute='name'. If
> clarity is what's desired, adding attribute= is perhaps what we should do
here.
> Having just case_sensitive= is misleading. 

Done.

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/MakeNames.cpp.tmpl:35: { "{{entry|cpp_name}}",
{{entry|cpp_name|hash}}, {{entry|cpp_name|length}} },
On 2014/09/26 06:09:46, Jens Widell wrote:
> I don't really understand why "|cpp_name" is used here instead of ".name".
> 
> The "|cpp_name" filter is the same as the "|script_name" filter unless there's
> an ImplementedAs= override. The "|script_name" filter returns
> os.path.basename(entry['name']). This may make sense when the name is the name
> of an interface or something like that, but for tag names, it seems
irrelevant.
> And *if* there was a tag with an ImplementedAs= for some reason, surely the
real
> name would be what should be used in this string literal in either case?
> 
> (Both implemented in Source/build/scripts/name_utilities.py.)

I am not expert on this, but it seems that while MakeQualifiedNames are fed with
lists of attributes and tags, MakeNames are fed with paths to implementions, so
it can be name="core/xml/XMLHttpRequestProgressEvent" ...
name="core/storage/StorageEvent". So you need something to get the name out of
that. I can't see anything in this code that cares about the paths but I guess
they are there for a reason. Or maybe someone just thought this was the best
layer to strip them if they are there as a side effect.

[Jens Widell, 2014-09-26 08:44:58]

On 2014/09/26 07:59:29, Daniel Bratell wrote:
> I've changed the sorting. Since it sorts something that isn't directly visible
> in the output data the results might still not be obvious to a human, but at
> least it is now well defined exactly what we sort (the name, path and all)
> instead of sorting dict objects.

The important things here are that the arrays are filled and enumerated in
consistent order, or the constants will point to the wrong names, and that the
order is stable across different runs of the script (so as to not get spurious
recompilations.)

Both look to be taken care of in this patch. (Not that they weren't before the
patch, of course.)

LGTM

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
File Source/build/scripts/templates/MakeNames.cpp.tmpl (right):

https://codereview.chromium.org/604743003/diff/60001/Source/build/scripts/tem...
Source/build/scripts/templates/MakeNames.cpp.tmpl:35: { "{{entry|cpp_name}}",
{{entry|cpp_name|hash}}, {{entry|cpp_name|length}} },
On 2014/09/26 07:59:29, Daniel Bratell wrote:
> On 2014/09/26 06:09:46, Jens Widell wrote:
> > I don't really understand why "|cpp_name" is used here instead of ".name".
> > 
> > The "|cpp_name" filter is the same as the "|script_name" filter unless
there's
> > an ImplementedAs= override. The "|script_name" filter returns
> > os.path.basename(entry['name']). This may make sense when the name is the
name
> > of an interface or something like that, but for tag names, it seems
> irrelevant.
> > And *if* there was a tag with an ImplementedAs= for some reason, surely the
> real
> > name would be what should be used in this string literal in either case?
> > 
> > (Both implemented in Source/build/scripts/name_utilities.py.)
> 
> I am not expert on this, but it seems that while MakeQualifiedNames are fed
with
> lists of attributes and tags, MakeNames are fed with paths to implementions,
so
> it can be name="core/xml/XMLHttpRequestProgressEvent" ...
> name="core/storage/StorageEvent". So you need something to get the name out of
> that. I can't see anything in this code that cares about the paths but I guess
> they are there for a reason. Or maybe someone just thought this was the best
> layer to strip them if they are there as a side effect.

Right. (I'm sorry I didn't fully investigate before commenting.)

Amended comment:

I think this ought to be "|script_name" rather than "|cpp_name". For interfaces,
the "C++ name" is usually the same as the actual interface name, but sometimes
has to be different for some reason. WebSocket seems to be one such case; it's
implemented C++-side by a class named DOMWebSocket. (This appears to be the only
such case that affects make_names.py and this template.)

In the resulting generated gen/blink/modules/EventTargetModulesNames.cpp file,
the interface name string is "DOMWebSocket", but the interface's name is
actually "WebSocket". Using "|script_name" here, we get "WebSocket" instead.

This mostly affects what DOMWebSocket::interfaceName() returns, which in turn,
AFAICT, only affects behavior in InspectorDOMDebuggerAgent::willHandleEvent(). I
don't know exactly how.

In short: this is probably a bug, but I don't think we should change it in this
CL.

[haraken, 2014-09-26 09:24:16]

LGTM

[Daniel Bratell, 2014-09-26 13:05:40]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-09-26 13:05:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/604743003/80001

[commit-bot: I haz the power, 2014-09-26 13:06:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-26 13:06:03]

Failed to apply patch for
Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl
  Hunk #2 FAILED at 17.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl.rej

Patch:       Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl
Index: Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl
diff --git a/Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl
b/Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl
index
958cc0f1828eca7c47914c398f4de458753f31c1..07ca9824683f69434a335ae6dca01a3945befc79
100644
--- a/Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl
+++ b/Source/build/scripts/templates/MakeQualifiedNames.cpp.tmpl
@@ -6,6 +6,7 @@
 #include "{{namespace}}Names.h"
 
 #include "wtf/StaticConstructors.h"
+#include "wtf/StdLibExtras.h"
 
 namespace blink {
 namespace {{namespace}}Names {
@@ -16,59 +17,89 @@ DEFINE_GLOBAL(AtomicString,
{{namespace_prefix}}NamespaceURI)
 
 {% if tags %}
 // Tags
-{% for tag in tags|sort %}
-DEFINE_GLOBAL({{namespace}}QualifiedName, {{tag|symbol}}Tag)
+
+void* {{suffix}}TagStorage[{{namespace}}TagsCount *
((sizeof({{namespace}}QualifiedName) + sizeof(void *) - 1) / sizeof(void *))];
+{% for tag in tags|sort(attribute='name', case_sensitive=True) %}
+const {{namespace}}QualifiedName& {{tag|symbol}}Tag =
reinterpret_cast<{{namespace}}QualifiedName*>(&{{suffix}}TagStorage)[{{loop.index0}}];
 {% endfor %}
 
 
 PassOwnPtr<const {{namespace}}QualifiedName*[]> get{{namespace}}Tags()
 {
     OwnPtr<const {{namespace}}QualifiedName*[]> tags = adoptArrayPtr(new const
{{namespace}}QualifiedName*[{{namespace}}TagsCount]);
-    {% for tag in tags|sort %}
-    tags[{{loop.index0}}] = reinterpret_cast<const
{{namespace}}QualifiedName*>(&{{tag|symbol}}Tag);
-    {% endfor %}
+    for (size_t i = 0; i < {{namespace}}TagsCount; i++)
+        tags[i] =
reinterpret_cast<{{namespace}}QualifiedName*>(&{{suffix}}TagStorage) + i;
     return tags.release();
 }
 
 {% endif %}
 // Attributes
-{% for attr in attrs|sort %}
-DEFINE_GLOBAL(QualifiedName, {{attr|symbol}}Attr)
+
+void* {{suffix}}AttrStorage[{{namespace}}AttrsCount * ((sizeof(QualifiedName) +
sizeof(void *) - 1) / sizeof(void *))];
+
+{% for attr in attrs|sort(attribute='name', case_sensitive=True) %}
+const QualifiedName& {{attr|symbol}}Attr =
reinterpret_cast<QualifiedName*>(&{{suffix}}AttrStorage)[{{loop.index0}}];
 {% endfor %}
 
 PassOwnPtr<const QualifiedName*[]> get{{namespace}}Attrs()
 {
     OwnPtr<const QualifiedName*[]> attrs = adoptArrayPtr(new const
QualifiedName*[{{namespace}}AttrsCount]);
-    {% for attr in attrs|sort %}
-    attrs[{{loop.index0}}] = reinterpret_cast<const
blink::QualifiedName*>(&{{attr|symbol}}Attr);
-    {% endfor %}
+    for (size_t i = 0; i < {{namespace}}AttrsCount; i++)
+        attrs[i] = reinterpret_cast<QualifiedName*>(&{{suffix}}AttrStorage) +
i;
     return attrs.release();
 }
 
 void init()
 {
+    struct NameEntry {
+        const char* name;
+        unsigned hash;
+        unsigned char length;
+        unsigned char isTag;
+        unsigned char isAttr;
+    };
+
     // Use placement new to initialize the globals.
     AtomicString {{namespace_prefix}}NS("{{namespace_uri}}",
AtomicString::ConstructFromLiteral);
 
     // Namespace
     new ((void*)&{{namespace_prefix}}NamespaceURI)
AtomicString({{namespace_prefix}}NS);
-    {% for name, tag_list in (tags + attrs)|groupby('name')|sort %}
-    StringImpl* {{tag_list[0]|symbol}}Impl =
StringImpl::createStatic("{{name}}", {{name|length}}, {{name|hash}});
-    {% endfor %}
-
-    // Tags
-    {% for tag in tags|sort %}
-    QualifiedName::createStatic((void*)&{{tag|symbol}}Tag, {{tag|symbol}}Impl,
{{namespace_prefix}}NS);
+    {% set tagnames = tags|map(attribute='name')|list() %}
+    {% set attrnames = attrs|map(attribute='name')|list() %}
+    static const NameEntry kNames[] = {
+    {% for name, tag_list in (tags + attrs)|groupby('name')|sort(attribute=0,
case_sensitive=True) %}
+        { "{{name}}", {{name|hash}}, {{name|length}}, {{ (name in tagnames)|int
}}, {{ (name in attrnames)|int }} },
     {% endfor %}
+    };
 
-    // Attrs
-    {% for attr in attrs|sort %}
-    {% if use_namespace_for_attrs %}
-    QualifiedName::createStatic((void*)&{{attr|symbol}}Attr,
{{attr|symbol}}Impl, {{namespace_prefix}}NS);
-    {% else %}
-    QualifiedName::createStatic((void*)&{{attr|symbol}}Attr,
{{attr|symbol}}Impl);
+    {% if tags %}
+    size_t tag_i = 0;
     {% endif %}
-    {% endfor %}
+    size_t attr_i = 0;
+    for (size_t i = 0; i < WTF_ARRAY_LENGTH(kNames); i++) {
+        StringImpl* stringImpl = StringImpl::createStatic(kNames[i].name,
kNames[i].length, kNames[i].hash);
+        {% if tags %}
+        if (kNames[i].isTag) {
+            void* address =
reinterpret_cast<{{namespace}}QualifiedName*>(&{{suffix}}TagStorage) + tag_i;
+            QualifiedName::createStatic(address, stringImpl,
{{namespace_prefix}}NS);
+            tag_i++;
+        }
+
+        if (!kNames[i].isAttr)
+            continue;
+        {% endif %}
+        void* address =
reinterpret_cast<QualifiedName*>(&{{suffix}}AttrStorage) + attr_i;
+        {% if use_namespace_for_attrs %}
+        QualifiedName::createStatic(address, stringImpl,
{{namespace_prefix}}NS);
+        {% else %}
+        QualifiedName::createStatic(address, stringImpl);
+        {% endif %}
+        attr_i++;
+    }
+    {% if tags %}
+    ASSERT(tag_i == {{namespace}}TagsCount);
+    {% endif %}
+    ASSERT(attr_i == {{namespace}}AttrsCount);
 }
 
 } // {{namespace}}

[Daniel Bratell, 2014-09-26 16:38:39]

The CQ bit was checked by bratell@opera.com

[Daniel Bratell, 2014-09-26 16:39:03]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-09-26 16:39:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/604743003/100001

[commit-bot: I haz the power, 2014-09-26 19:29:56]

Committed patchset #4 (id:100001) as 182780