Bug 217916 - [JSC] OrdinarySet should invoke custom [[Set]] methods
Summary: [JSC] OrdinarySet should invoke custom [[Set]] methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-19 11:40 PDT by Alexey Shvayka
Modified: 2021-04-26 08:21 PDT (History)
20 users (show)

See Also:


Attachments
Patch (80.22 KB, patch)
2020-10-19 12:58 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
WIP (82.15 KB, patch)
2021-01-13 08:04 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
WIP (93.02 KB, patch)
2021-01-18 12:40 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
WIP (198.89 KB, patch)
2021-01-28 08:45 PST, Alexey Shvayka
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP (198.03 KB, patch)
2021-01-28 09:18 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
WIP (118.55 KB, patch)
2021-04-09 19:16 PDT, Alexey Shvayka
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP (118.57 KB, patch)
2021-04-09 19:30 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (164.29 KB, patch)
2021-04-14 14:47 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (171.54 KB, patch)
2021-04-21 04:04 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (178.54 KB, patch)
2021-04-23 09:10 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (178.54 KB, patch)
2021-04-26 04:05 PDT, Alexey Shvayka
shvaikalesh: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2020-10-19 11:40:45 PDT
Introduce OverridesPut[ByIndex] type info flags
Comment 1 Alexey Shvayka 2020-10-19 12:58:29 PDT
Created attachment 411780 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-10-22 12:15:49 PDT
<rdar://problem/70583959>
Comment 3 Yusuke Suzuki 2020-10-22 13:10:28 PDT
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.
Comment 4 Alexey Shvayka 2021-01-13 08:04:08 PST Comment hidden (obsolete)
Comment 5 Alexey Shvayka 2021-01-18 12:40:09 PST Comment hidden (obsolete)
Comment 6 Alexey Shvayka 2021-01-28 08:45:37 PST Comment hidden (obsolete)
Comment 7 Alexey Shvayka 2021-01-28 09:18:45 PST Comment hidden (obsolete)
Comment 8 Alexey Shvayka 2021-04-09 19:16:14 PDT Comment hidden (obsolete)
Comment 9 Alexey Shvayka 2021-04-09 19:30:33 PDT Comment hidden (obsolete)
Comment 10 Alexey Shvayka 2021-04-14 14:47:30 PDT Comment hidden (obsolete)
Comment 11 EWS Watchlist 2021-04-14 14:48:23 PDT
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
Comment 12 Alexey Shvayka 2021-04-15 09:10:06 PDT
(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
Comment 13 Yusuke Suzuki 2021-04-16 03:28:49 PDT
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
Comment 14 Alexey Shvayka 2021-04-21 04:04:16 PDT
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.
Comment 15 Alexey Shvayka 2021-04-21 04:04:41 PDT
(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.
Comment 16 Alexey Shvayka 2021-04-23 09:10:07 PDT
Created attachment 426914 [details]
Patch

Fix, optimize, and expand test coverage for reified static properties.
Comment 17 Alexey Shvayka 2021-04-26 04:05:42 PDT
Created attachment 427034 [details]
Patch

Adjust property attributes in JSDollarVM.
Comment 18 Alexey Shvayka 2021-04-26 07:47:41 PDT
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).
Comment 19 Alexey Shvayka 2021-04-26 08:21:14 PDT
Committed r276592 (237028@main): <https://commits.webkit.org/237028@main>