Bug 217916 – [JSC] OrdinarySet should invoke custom [[Set]] methods

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

[Alexey Shvayka]

Introduce OverridesPut[ByIndex] type info flags

[Alexey Shvayka]

Created attachment 411780 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/70583959>

[Yusuke Suzuki]

Comment on attachment 411780 [details]
Patch

Can we have a microbenchmark which illustrates the effect of this change for ClonedArguments? And can you measure the perf effect on that? ClonedArguments is used when `arguments` in strict mode, so, it is one of the frequently used critical object. We need to keep performance for this.

[Alexey Shvayka]

Created attachment 417532 [details]
WIP

[Alexey Shvayka]

Created attachment 417843 [details]
WIP

Switch back to OverridesPut approach.

[Alexey Shvayka]

Created attachment 418648 [details]
WIP

Combine OverridesPut with previous approach of calling [[GetOwnProperty]] overrides in putInlineSlot().

[Alexey Shvayka]

Created attachment 418650 [details]
WIP

[Alexey Shvayka]

Created attachment 425673 [details]
WIP

[Alexey Shvayka]

Created attachment 425674 [details]
WIP

Fix build.

[Alexey Shvayka]

Created attachment 426047 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 411780 [details]
> Patch
> 
> Can we have a microbenchmark which illustrates the effect of this change for
> ClonedArguments? And can you measure the perf effect on that?
> ClonedArguments is used when `arguments` in strict mode, so, it is one of
> the frequently used critical object. We need to keep performance for this.

I've deferred fixing putByIndex() to a follow-up; new patch fixes only non-index put(), with following microbenchmark results:

                                                 trunk                      patch

put-slow-no-cache-function                   59.2466+-1.0115     ?     59.6670+-0.8626        ?
put-slow-no-cache-js-proxy                   59.0333+-0.7585     ^     50.2321+-0.9472        ^ definitely 1.1752x faster
put-slow-no-cache-long-prototype-chain       49.9765+-1.1316     ^     47.0827+-1.0213        ^ definitely 1.0615x faster
put-slow-no-cache                            51.4902+-0.7962     ^     49.5543+-0.9953        ^ definitely 1.0391x faster
put-slow-no-cache-array                      53.3368+-0.8638     ?     53.8900+-0.6450        ? might be 1.0104x slower
property-replace-and-setter-on-js-proxy     223.9187+-3.8382     ?    225.2152+-3.6012        ?
reflect-set-with-receiver                   131.7529+-1.5210     ^     56.4580+-1.2031        ^ definitely 2.3336x faster

[Yusuke Suzuki]

Comment on attachment 426047 [details]
Patch

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

This is awesome. r=me

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:229
> +    RELEASE_AND_RETURN(scope, obj->methodTable(vm)->put(obj, globalObject, propertyName, value, slot));

This is a bit incorrect, but the incorrectness is unobservable.

"Hello".length = 42;

This should be an error when we are assigning this value to StringObject materialized from "Hello". But we avoid materializing it (that's important optimization), and we are querying to StringPrototype first.
And since StringPrototype has the same "length" property, we are anyway throwing an error.

Can we have an explicit test for this? ("Hello".length = 42 in sloppy and strict modes) And can we have FIXME comment about it here?

> Source/JavaScriptCore/runtime/JSObject.cpp:823
> +                customSetter(globalObject, JSValue::encode(slot.thisValue()), JSValue::encode(value), propertyName);
>                  RETURN_IF_EXCEPTION(scope, false);
> -                if (result != TriState::Indeterminate)
> -                    return result == TriState::True;
> +                return true;

Let's make it

scope.release();
customSetter(globalObject, JSValue::encode(slot.thisValue()), JSValue::encode(value), propertyName);
return true;

> Source/JavaScriptCore/runtime/JSObject.cpp:831
> +                    RELEASE_AND_RETURN(scope, customSetter(globalObject, JSValue::encode(obj), JSValue::encode(value), propertyName));

Should we return true too?

> Source/JavaScriptCore/runtime/JSObject.h:-1441
> -
> -ALWAYS_INLINE void JSObject::doPutPropertySecurityCheck(JSObject*, JSGlobalObject*, PropertyName, PutPropertySlot&)
> -{
> -}

This is awesome.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:70
> +        if (structure->hasReadOnlyOrGetterSetterPropertiesExcludingProto() || structure->typeInfo().overridesPut() || structure->typeInfo().overridesGetPrototype())

One optimization idea I think. Should we check `structure->typeInfo().overridesPut()` even for the originating object?
Let's consider a case,

[A: object, which overrides put] -[[Prototype]]-> [B: object, which does not override put]

In that case, when calling A::put, we are already in A::put. And then we can consider whether we can use fast-path put or not. At that point, I think we do not need to check OverridesPut for the initial object since we already called A::put, is my understanding correct?
RegExpObject, ErrorInstance, JSFunction, TypedArrays, and Arguments objects are the cases.

(If we do this optimization, we also need to take care for Object.assign case since that function directly calls it.)

> Source/JavaScriptCore/runtime/PutPropertySlot.h:119
> +    bool isTaintedByOpaqueObject() const { return m_isTaintedByOpaqueObject; }

Can we remove `Context context() const { return static_cast<Context>(m_context); }`'s cast?

> Source/JavaScriptCore/runtime/PutPropertySlot.h:140
> +    bool m_isTaintedByOpaqueObject { false };

Can we make it a bit? (`bool m_isTaintedByOpaqueObject : 1`). And we need to put `false` initialization in the constructor.

> Source/JavaScriptCore/runtime/PutPropertySlot.h:142
>      uint8_t m_context;

Can we use Context instead of uint8_t?

> Source/JavaScriptCore/runtime/StringPrototype.cpp:-168
> -    putDirectWithoutTransition(vm, vm.propertyNames->length, jsNumber(0), PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum);

Nice

[Alexey Shvayka]

Created attachment 426668 [details]
Patch

Add string "length" check & test, implement canPerformFastPutInline() optimization, fix JSGlobalObject::put() to stop prototype chain traversal, introduce putInlineFastReplacingStaticPropertyIfNeeded(), and tweak PutPropertySlot.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #13)
> This is awesome. r=me

Many thanks for reviewing this, Yusuke!
Your comments were extremely helpful and led me to discovering a several bugs.

> > Source/JavaScriptCore/runtime/JSCJSValue.cpp:229
> > +    RELEASE_AND_RETURN(scope, obj->methodTable(vm)->put(obj, globalObject, propertyName, value, slot));
> 
> Can we have an explicit test for this? ("Hello".length = 42 in sloppy and strict modes)

Added JSTests/stress/put-to-primitive.js

> And can we have FIXME comment about it here?

I've added an inline check for string & "length" instead, which is fine considering new primitive wrappers are unlikely to be exotic.

> > Source/JavaScriptCore/runtime/JSObject.cpp:823
> > +                customSetter(globalObject, JSValue::encode(slot.thisValue()), JSValue::encode(value), propertyName);
> >                  RETURN_IF_EXCEPTION(scope, false);
> > -                if (result != TriState::Indeterminate)
> > -                    return result == TriState::True;
> > +                return true;
> 
> Let's make it
> 
> scope.release();
> customSetter(globalObject, JSValue::encode(slot.thisValue()), JSValue::encode(value), propertyName);
> return true;

Fixed (and for JSRemoteDOMWindow::put() as well).
Nice, it's now aligned with userland Accessor.

> > Source/JavaScriptCore/runtime/JSObject.cpp:831
> > +                    RELEASE_AND_RETURN(scope, customSetter(globalObject, JSValue::encode(obj), JSValue::encode(value), propertyName));
> 
> Should we return true too?

I don't think so: a CustomValue handles writability on its own, without toggling PropertyAttribute::ReadOnly (for performance reasons),
so it may return `false` + throw an exception. A good example of that is RegExp's "lastIndex" setter.

> > Source/JavaScriptCore/runtime/JSObjectInlines.h:70
> > +        if (structure->hasReadOnlyOrGetterSetterPropertiesExcludingProto() || structure->typeInfo().overridesPut() || structure->typeInfo().overridesGetPrototype())
> 
> At that point, I think we do not need to check OverridesPut for the initial object since we already called A::put, is my understanding correct?

Yes, absolutely. putInlineSlow() has the same assumption: once ordinary put() is reached, all special properties were already handled.
This is a great suggestion that led me to discovering two flaws in handling of non-reified static properties:
  1. We can't handle them only in putInlineSlow() since an object may not have read-only properties / accessors / put() override.
  2. Reflect.set called on receiver should also correctly handle non-reified static functions (preserve attributes) / accessors (throw).

putInlineFastReplacingStaticPropertyIfNeeded() was introduced to fix those; test coverage was expanded.

> RegExpObject, ErrorInstance, JSFunction, TypedArrays, and Arguments objects are the cases.

This will benefit pretty much all built-ins by using putInlineFast() at least for %BuiltIn%.prototype -> Object.prototype segment of the [[Set]].
Most built-ins' prototypes are exotic or have read-only symbols though.

> (If we do this optimization, we also need to take care for Object.assign case since that function directly calls it.)

Object.assign calls canPerformFastPutInline() only on JSFinalObjects, so no tweaking required.

> > Source/JavaScriptCore/runtime/PutPropertySlot.h:119
> > +    bool isTaintedByOpaqueObject() const { return m_isTaintedByOpaqueObject; }
> 
> Can we remove `Context context() const { return static_cast<Context>(m_context); }`'s cast?

Removed.

> > Source/JavaScriptCore/runtime/PutPropertySlot.h:140
> > +    bool m_isTaintedByOpaqueObject { false };
> 
> Can we make it a bit? (`bool m_isTaintedByOpaqueObject : 1`). And we need to
> put `false` initialization in the constructor.

Fixed.

> > Source/JavaScriptCore/runtime/PutPropertySlot.h:142
> >      uint8_t m_context;
> 
> Can we use Context instead of uint8_t?

Fixed.

[Alexey Shvayka]

Created attachment 426914 [details]
Patch

Fix, optimize, and expand test coverage for reified static properties.

[Alexey Shvayka]

Created attachment 427034 [details]
Patch

Adjust property attributes in JSDollarVM.

[Alexey Shvayka]

Comment on attachment 427034 [details]
Patch

run-layout-tests-in-guard-malloc-stress-mode failure doesn't reproduce locally; it's probably an issue similar to https://bugs.webkit.org/show_bug.cgi?id=224358.

Putting r+ on my own patch to make WPT export bot happy (it ignores reviewed patches that are obsolete).

[Alexey Shvayka]

Committed r276592 (237028@main): <https://commits.webkit.org/237028@main>