[webkit-reviews] review granted: [Bug 195498] [JSC] Reduce # of structures in JSGlobalObject initialization : [Attachment 364106] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 195498: [JSC] Reduce # of structures in JSGlobalObject initialization
https://bugs.webkit.org/show_bug.cgi?id=195498

Attachment 364106: Patch

https://bugs.webkit.org/attachment.cgi?id=364106&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 364106
  --> https://bugs.webkit.org/attachment.cgi?id=364106
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364106&action=review

I presume there is enough regression test coverage of these things that we have
some good confidence we got them all right.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:130
> +    const Identifier unscopableNames[] = {

There is some reference count churn initializing an array of Identifier. Might
be nice to look for an idiom with less wasted work. For example, it would be
slightly more efficient to make an array of const Identifier* and then
dereference all of them to avoid 9 ref/deref pairs.

> Source/JavaScriptCore/runtime/VM.h:572
> +    Structure* setIteratorStructure()
> +    {
> +	   if (LIKELY(m_setIteratorStructure))
> +	       return m_setIteratorStructure.get();
> +	   return setIteratorStructureSlow();
> +    }

I prefer an idiom where we don’t have so many multiline function bodies in a
class definition. For me they interfere with my ability to use the class
definition to get an overview of what’s in the class. The pattern is also easy
to avoid by moving inlines function bodies down in the header, outside the
class definition, but with the cost of not being able to see immediately what
each function does by seeing its implementation. But it seems that VM already
has tons of these. So I don’t think more are going to make things worse.