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