Bug 179734 – It should be easier to reify lazy property names

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

We reify lazy property names in a few different ways, each specific to the JSCell implementation, in put() instead of having a special function to do reification. Let's make that simpler.

[JF Bastien]

<rdar://problem/35492521>

[JF Bastien]

Created attachment 326996 [details]
patch

[Build Bot]

Attachment 326996 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSFunction.h:193:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 326996 [details]
patch

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

> Source/JavaScriptCore/runtime/JSObjectInlines.h:267
> +    switch (methodTable(vm)->reifyPropertyNameIfNeeded(this, globalObject(vm)->globalExec(), propertyName)) {
> +    case PropertyNameReified::Nothing: break;
> +    case PropertyNameReified::Something: break;
> +    case PropertyNameReified::TriedButFailed: RELEASE_ASSERT_NOT_REACHED();
> +    }

Though I like this style for conciseness, I think the WebKit way is to have the break statement and assert on a different line.

[Mark Lam]

Comment on attachment 326996 [details]
patch

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

> Source/JavaScriptCore/runtime/ClassInfo.h:41
> -    typedef void (*DestroyFunctionPtr)(JSCell*);
> +    using DestroyFunctionPtr = void (*)(JSCell*);

I suggest doing these refactoring in a subsequent patch.  It is actually not essential to this patch.

> Source/JavaScriptCore/runtime/JSCell.h:63
> +enum class PropertyNameReified {

nit: I think PropertyReificationResult is a better name.

> Source/JavaScriptCore/runtime/JSFunction.h:192
> +    enum class LazyProperty

I think a better name for this is "Laziness" or ""LazinessState".  "LazyProperty" implies a JS property which can add confusion.

> Source/JavaScriptCore/runtime/JSFunction.h:196
> +        IsLazyReified,

I suggest simply naming this "IsReified".  "IsLazyReified" has this ambiguity about whether it has been reified yet when you do mean that it is definitely reified.  The fact that it is "reified" implies that it was once lazy.  So, "IsReified" is sufficient and clearer IMHO.

>> Source/JavaScriptCore/runtime/JSObjectInlines.h:267
>> +    switch (methodTable(vm)->reifyPropertyNameIfNeeded(this, globalObject(vm)->globalExec(), propertyName)) {
>> +    case PropertyNameReified::Nothing: break;
>> +    case PropertyNameReified::Something: break;
>> +    case PropertyNameReified::TriedButFailed: RELEASE_ASSERT_NOT_REACHED();
>> +    }
> 
> Though I like this style for conciseness, I think the WebKit way is to have the break statement and assert on a different line.

Hmmm, one of the downside of this generic approach is that it burdens all objects with a  reifyPropertyNameIfNeeded() check even when only a few sub-classes need it.  Is it possible to avoid this overhead?  Or is the overhead insignificant?

If perf is an issue, maybe you can check a flag (e.g. HasLazilyReifiedProperties) before choosing to do the virtual dispatch.  See use of the OverridesGetPropertyNames attribute.

[JF Bastien]

Created attachment 327017 [details]
patch

Changes suggested by Mark. Will run release perf numbers to see if it's worth doing the last suggestion.

[Build Bot]

Attachment 327017 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSFunction.h:193:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 327073 [details]
results

Perf results are "might be 1.0035x slower". Nothing really stands out, seems to be in the noise.

[JF Bastien]

Created attachment 327075 [details]
patch

Fix style nit.

[Keith Miller]

Comment on attachment 327075 [details]
patch

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

r=me.

> Source/JavaScriptCore/runtime/JSFunction.h:204
> +    enum class PropertyStatus {
> +        Eager,
> +        Lazy,
> +        Reified,
> +    };
> +    static bool isLazy(PropertyStatus property) { return property == PropertyStatus::Lazy || property == PropertyStatus::Reified; }
> +    static bool isReified(PropertyStatus property) { return property == PropertyStatus::Reified; }
> +
> +    PropertyStatus reifyLazyPropertyIfNeeded(VM&, ExecState*, PropertyName);
> +    PropertyStatus reifyLazyPropertyForHostOrBuiltinIfNeeded(VM&, ExecState*, PropertyName);
> +    PropertyStatus reifyLazyLengthIfNeeded(VM&, ExecState*, PropertyName);
> +    PropertyStatus reifyLazyNameIfNeeded(VM&, ExecState*, PropertyName);
> +    PropertyStatus reifyLazyBoundNameIfNeeded(VM&, ExecState*, PropertyName);

It seems a little silly that we have all this in addition to the method table functionality. Can you file a bug to clean this up later?

[JF Bastien]

(In reply to Keith Miller from comment #10)
> Comment on attachment 327075 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327075&action=review
> 
> r=me.
> 
> > Source/JavaScriptCore/runtime/JSFunction.h:204
> > +    enum class PropertyStatus {
> > +        Eager,
> > +        Lazy,
> > +        Reified,
> > +    };
> > +    static bool isLazy(PropertyStatus property) { return property == PropertyStatus::Lazy || property == PropertyStatus::Reified; }
> > +    static bool isReified(PropertyStatus property) { return property == PropertyStatus::Reified; }
> > +
> > +    PropertyStatus reifyLazyPropertyIfNeeded(VM&, ExecState*, PropertyName);
> > +    PropertyStatus reifyLazyPropertyForHostOrBuiltinIfNeeded(VM&, ExecState*, PropertyName);
> > +    PropertyStatus reifyLazyLengthIfNeeded(VM&, ExecState*, PropertyName);
> > +    PropertyStatus reifyLazyNameIfNeeded(VM&, ExecState*, PropertyName);
> > +    PropertyStatus reifyLazyBoundNameIfNeeded(VM&, ExecState*, PropertyName);
> 
> It seems a little silly that we have all this in addition to the method
> table functionality. Can you file a bug to clean this up later?

I filed  radar 35592139 as a follow-up. I think we have a few other things that we can clean up!

[WebKit Commit Bot]

Comment on attachment 327075 [details]
patch

Clearing flags on attachment: 327075

Committed r224927: <https://trac.webkit.org/changeset/224927>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 327075 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:24
> +        We reify lazy property names in a few different ways, each
> +        specific to the JSCell implementation, in put() instead of having
> +        a special function to do reification. Let's make that simpler.
> +
> +        This patch makes it easier to reify property names in a uniform
> +        manner, and does so in JSFunction. As a follow up I'll use the
> +        same mechanics for:
> +
> +        ClonedArguments   callee, iteratorSymbol (Symbol.iterator)
> +        ErrorConstructor  stackTraceLimit
> +        ErrorInstance     line, column, sourceURL, stack
> +        GenericArguments  length, callee, iteratorSymbol (Symbol.iterator)
> +        GetterSetter      RELEASE_ASSERT_NOT_REACHED()
> +        JSArray           length
> +        RegExpObject      lastIndex
> +        StringObject      length

I think if we truly did a generic form of lazy properties, it would mean we need to logically call your vtable function on get(). This would surely lead to slow downs. There are probably ways to make it work, bit it requires monitoring everyone that calls into the various property MethodTable hooks to make sure they first reifyLazyPropertyIfNeeded. This design feels untenable since we call into the method table in many places that are not just in JSObject.

> Source/JavaScriptCore/runtime/JSCell.h:67
> +enum class PropertyReificationResult {
> +    Nothing,
> +    Something,
> +    TriedButFailed, // Sometimes the property name already exists but has special behavior and can't be reified, e.g. Array.length.
> +};

I don't see a need to have this.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:267
> +    switch (methodTable(vm)->reifyPropertyNameIfNeeded(this, globalObject(vm)->globalExec(), propertyName)) {
> +    case PropertyReificationResult::Nothing: break;
> +    case PropertyReificationResult::Something: break;
> +    case PropertyReificationResult::TriedButFailed: RELEASE_ASSERT_NOT_REACHED();
> +    }

This feels like the wrong place to have this to me. Until we have more evidence, I would have just done this inside put_getter_setter_by_id

[Alexey Shvayka]

*** Bug 157148 has been marked as a duplicate of this bug. ***