Bug 216061 – [JSC] Cache toString / valueOf / @@toPrimitive for major cases

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

[Yusuke Suzuki]

...

[Radar WebKit Bug Importer]

<rdar://problem/68179682>

[Yusuke Suzuki]

Created attachment 407743 [details]
Patch

[Yusuke Suzuki]

Created attachment 407746 [details]
Patch

[Yusuke Suzuki]

Created attachment 407747 [details]
Patch

[Yusuke Suzuki]

Created attachment 407811 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 407811 [details]
Patch

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

> Source/JavaScriptCore/bytecode/Watchpoint.h:117
> +    macro(ObjectToStringAdaptiveStructure, CachedSpecialPropertyAdaptiveStructureWatchpoint)

Will change.

[Saam Barati]

Comment on attachment 407811 [details]
Patch

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

Nice. r=me

> Source/JavaScriptCore/runtime/CachedSpecialPropertyAdaptiveStructureWatchpoint.h:2
> + * Copyright (C) 2019 Apple Inc. All rights reserved.

2020

> Source/JavaScriptCore/runtime/StructureRareData.cpp:112
> +    WTF::storeStoreFence(); // Store to the field of this struct should be visible to the concurrent collectors after struct is visible.

from what I said on slack, I don't think this is needed, but double check

> Source/JavaScriptCore/runtime/StructureRareData.cpp:184
> +            cache.m_presentWatchpoint = makeUnique<CachedSpecialPropertyAdaptiveInferredPropertyValueWatchpoint>(equivCondition, this);

let's call "m_presentWatchpoint" m_equivalenceWatchpoint or something similar

> Source/JavaScriptCore/runtime/StructureRareData.cpp:219
> +                    continue;

continue isn't needed. Did you mean break so we continue the outer loop?

> Source/JavaScriptCore/runtime/StructureRareDataInlines.h:39
> +    Bag<CachedSpecialPropertyAdaptiveStructureWatchpoint> m_missWatchpoints;
> +    std::unique_ptr<CachedSpecialPropertyAdaptiveInferredPropertyValueWatchpoint> m_presentWatchpoint;

maybe in the future we can use ObjectPropertyConditionSet? Or is this just to save memory?

[Saam Barati]

Comment on attachment 407811 [details]
Patch

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

>> Source/JavaScriptCore/runtime/CachedSpecialPropertyAdaptiveStructureWatchpoint.h:2
>> + * Copyright (C) 2019 Apple Inc. All rights reserved.
> 
> 2020

I see you renamed this. Maybe 2019-2020

[Yusuke Suzuki]

Comment on attachment 407811 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/bytecode/Watchpoint.h:117
>> +    macro(ObjectToStringAdaptiveStructure, CachedSpecialPropertyAdaptiveStructureWatchpoint)
> 
> Will change.

Fixed.

>>> Source/JavaScriptCore/runtime/CachedSpecialPropertyAdaptiveStructureWatchpoint.h:2
>>> + * Copyright (C) 2019 Apple Inc. All rights reserved.
>> 
>> 2020
> 
> I see you renamed this. Maybe 2019-2020

Fixed.

>> Source/JavaScriptCore/runtime/StructureRareData.cpp:112
>> +    WTF::storeStoreFence(); // Store to the field of this struct should be visible to the concurrent collectors after struct is visible.
> 
> from what I said on slack, I don't think this is needed, but double check

Yeah, agree. Removed.

>> Source/JavaScriptCore/runtime/StructureRareData.cpp:184
>> +            cache.m_presentWatchpoint = makeUnique<CachedSpecialPropertyAdaptiveInferredPropertyValueWatchpoint>(equivCondition, this);
> 
> let's call "m_presentWatchpoint" m_equivalenceWatchpoint or something similar

Fixed.

>> Source/JavaScriptCore/runtime/StructureRareData.cpp:219
>> +                    continue;
> 
> continue isn't needed. Did you mean break so we continue the outer loop?

Oops! Right. To simply, I'll extract this as clearCacheIfInvalidated lambda, and return instead of continue.

>> Source/JavaScriptCore/runtime/StructureRareDataInlines.h:39
>> +    std::unique_ptr<CachedSpecialPropertyAdaptiveInferredPropertyValueWatchpoint> m_presentWatchpoint;
> 
> maybe in the future we can use ObjectPropertyConditionSet? Or is this just to save memory?

Sounds good. I'll file a bug and paste a FIXME here.

[Yusuke Suzuki]

Created attachment 407845 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Comment on attachment 407845 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:-2208
> -    // Make sure that whatever default value methods there are on object's prototype chain are
> -    // being watched.
> -    for (const JSObject* object = this; object; object = object->structure(vm)->storedPrototypeObject(object))
> -        object->structure(vm)->startWatchingInternalPropertiesIfNecessary(vm);
> -

I revert this for now. The reason is the following,

1. DFG Graph::canOptimizeStringObjectAccess is checking both valueOf and toString status
2. However, in the normal path, valueOf of StringObject is rarely accessed (toString suceeeds, and we do not access to valueOf).
3. As a result, valueOf related structures are not watched at all. So in DFG, DFG Graph::canOptimizeStringObjectAccess failed.

This affects on JetStream2/date-format-xparb-SP. For now, I'll keep this.

[Yusuke Suzuki]

Comment on attachment 407845 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSObject.cpp:-2208
>> -
> 
> I revert this for now. The reason is the following,
> 
> 1. DFG Graph::canOptimizeStringObjectAccess is checking both valueOf and toString status
> 2. However, in the normal path, valueOf of StringObject is rarely accessed (toString suceeeds, and we do not access to valueOf).
> 3. As a result, valueOf related structures are not watched at all. So in DFG, DFG Graph::canOptimizeStringObjectAccess failed.
> 
> This affects on JetStream2/date-format-xparb-SP. For now, I'll keep this.

Ah, no. Yeah, since this is not invoked from IC, basically, without this, we do not put watchpoints. So, as a result, PropertyCondition::isWatchableWhenValid for Equavalent will fail.

[Yusuke Suzuki]

Comment on attachment 407845 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSObject.cpp:-2208
>>> -
>> 
>> I revert this for now. The reason is the following,
>> 
>> 1. DFG Graph::canOptimizeStringObjectAccess is checking both valueOf and toString status
>> 2. However, in the normal path, valueOf of StringObject is rarely accessed (toString suceeeds, and we do not access to valueOf).
>> 3. As a result, valueOf related structures are not watched at all. So in DFG, DFG Graph::canOptimizeStringObjectAccess failed.
>> 
>> This affects on JetStream2/date-format-xparb-SP. For now, I'll keep this.
> 
> Ah, no. Yeah, since this is not invoked from IC, basically, without this, we do not put watchpoints. So, as a result, PropertyCondition::isWatchableWhenValid for Equavalent will fail.

And in this path, we will put this watchpoint for toString, but not for valueOf since we are not accessing it at runtime actually. Then, Graph::canOptimizeStringObjectAccess almost always fail.
We should do in the meantime.

[Yusuke Suzuki]

Comment on attachment 407845 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/JSObject.cpp:-2208
>>>> -
>>> 
>>> I revert this for now. The reason is the following,
>>> 
>>> 1. DFG Graph::canOptimizeStringObjectAccess is checking both valueOf and toString status
>>> 2. However, in the normal path, valueOf of StringObject is rarely accessed (toString suceeeds, and we do not access to valueOf).
>>> 3. As a result, valueOf related structures are not watched at all. So in DFG, DFG Graph::canOptimizeStringObjectAccess failed.
>>> 
>>> This affects on JetStream2/date-format-xparb-SP. For now, I'll keep this.
>> 
>> Ah, no. Yeah, since this is not invoked from IC, basically, without this, we do not put watchpoints. So, as a result, PropertyCondition::isWatchableWhenValid for Equavalent will fail.
> 
> And in this path, we will put this watchpoint for toString, but not for valueOf since we are not accessing it at runtime actually. Then, Graph::canOptimizeStringObjectAccess almost always fail.
> We should do in the meantime.

Ideally, DFG should not check toString & valueOf. DFG should check either of that by checking the context. And after that, we can safely remove it since our new code already puts watchpoints appropriately.

[Yusuke Suzuki]

Comment on attachment 407845 [details]
Patch

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

>>>>> Source/JavaScriptCore/runtime/JSObject.cpp:-2208
>>>>> -
>>>> 
>>>> I revert this for now. The reason is the following,
>>>> 
>>>> 1. DFG Graph::canOptimizeStringObjectAccess is checking both valueOf and toString status
>>>> 2. However, in the normal path, valueOf of StringObject is rarely accessed (toString suceeeds, and we do not access to valueOf).
>>>> 3. As a result, valueOf related structures are not watched at all. So in DFG, DFG Graph::canOptimizeStringObjectAccess failed.
>>>> 
>>>> This affects on JetStream2/date-format-xparb-SP. For now, I'll keep this.
>>> 
>>> Ah, no. Yeah, since this is not invoked from IC, basically, without this, we do not put watchpoints. So, as a result, PropertyCondition::isWatchableWhenValid for Equavalent will fail.
>> 
>> And in this path, we will put this watchpoint for toString, but not for valueOf since we are not accessing it at runtime actually. Then, Graph::canOptimizeStringObjectAccess almost always fail.
>> We should do in the meantime.
> 
> Ideally, DFG should not check toString & valueOf. DFG should check either of that by checking the context. And after that, we can safely remove it since our new code already puts watchpoints appropriately.

Filed https://bugs.webkit.org/show_bug.cgi?id=216117
Maybe, I'll do that for StringObject first since it is super simple.

[Yusuke Suzuki]

Created attachment 407871 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Committed r266567: <https://trac.webkit.org/changeset/266567>

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #17)
> Committed r266567: <https://trac.webkit.org/changeset/266567>

JSTests/test262/test/language/expressions/dynamic-import/custom-primitive.js started to fail: https://build.webkit.org/builders/Apple-Catalina-Debug-Test262-Tests/builds/7201.

[Yusuke Suzuki]

(In reply to Alexey Shvayka from comment #18)
> (In reply to Yusuke Suzuki from comment #17)
> > Committed r266567: <https://trac.webkit.org/changeset/266567>
> 
> JSTests/test262/test/language/expressions/dynamic-import/custom-primitive.js
> started to fail:
> https://build.webkit.org/builders/Apple-Catalina-Debug-Test262-Tests/builds/
> 7201.

Will be fixed in https://bugs.webkit.org/show_bug.cgi?id=216222