Bug 216061

<rdar://problem/68179682>

Created attachment 407743 [details]
Patch

Created attachment 407746 [details]
Patch

Created attachment 407747 [details]
Patch

Created attachment 407811 [details]
Patch

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.

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?

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

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.

Created attachment 407845 [details]
Patch

Patch for landing

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.

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.

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.

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.

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.

Created attachment 407871 [details]
Patch

Patch for landing

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

(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.

(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