[webkit-reviews] review granted: [Bug 158557] [WebIDL] Don't eagerly reify constructor and prototype properties : [Attachment 428167] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 158557: [WebIDL] Don't eagerly reify constructor and prototype properties
https://bugs.webkit.org/show_bug.cgi?id=158557

Attachment 428167: Patch

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




--- Comment #40 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 428167
  --> https://bugs.webkit.org/attachment.cgi?id=428167
Patch

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

LGTM, just a few comments.

> Source/JavaScriptCore/ChangeLog:16
> +	   2. Enables LazyPropertyCallback to return GetterSetter /
CustomGetterSetter, ensuring
> +	      correct structure flags and slot initialization.

What was it used for before? Might be worth stating that here

> Source/JavaScriptCore/runtime/JSObject.cpp:795
> +		       slot.disableCaching();

I wonder if long term we could have a way of telling the IC to look at the new
result from the structure lookup on the LOC below this. Right now, if we see a
different structure, we might assume a Transition. Maybe we could find a way to
tell the IC to ignore this reification just once. Might not be important now,
but maybe longer term, it would be a nice thing to do.

> Source/JavaScriptCore/runtime/JSObject.cpp:796
> +		       return
lookupPropertyForPut<LookupKind::StructureOnly>(vm, object,
object->structure(vm), propertyName, slot);

nit:

I think a nicer way to do this would be to just make the structure lookup a
lambda. Do away with this LookupKind enum. The main caller of this function
shouldn't worry about this implementation detail.

Then here, instead of recursing with a different enum, you can just return the
invocation of the structure lookup lambda.

> Source/JavaScriptCore/runtime/JSObject.cpp:798
> +		   // FIXME: Remove this after writable accessors are
introduced to static hash tables.

not sure we really need this FIXME anymore

> Source/JavaScriptCore/runtime/Lookup.h:41
> +// FIXME: Templatize HashTable and CompactHashIndex so int8_t sized values
can be used for small tables to reduce binary size.

link a bugzilla

> Source/JavaScriptCore/runtime/Lookup.h:109
> +	   auto isEnabledCallback =
reinterpret_cast<IsLazyPropertyEnabledCallback>(m_values.value2);

bitwise_cast

> Source/WebCore/ChangeLog:22
> +	   Another important advantage: a feature that was enabled after its
interface was created,
> +	   immediately becomes usable (no page reload needed).

except for inline caching?