We just cache these without caring what happens with the prototype that holds it. So we could delete the property, and we'd still execute the IC as if the custom value/accessor were still present
Custom values broken: ``` function assert(b) { if (!b) throw new Error; } function getMultiline(o) { return o.multiline; } noInline(getMultiline); const o = {}; o.__proto__ = RegExp; RegExp.multiline = false; for (let i = 0; i < 500; ++i) { assert(getMultiline(o) === false); } delete RegExp.input; delete RegExp.multiline; assert(getMultiline(o) === undefined); // fail here ``` Custom accessors broken: ``` function bar(e) { return e.nodeType; } let node = ...; for (let i = 0; i < 500; ++i) assert(bar(node) === 1); assert(delete Node.prototype.nodeType); assert(bar(node) === undefined); // fail here ```
Created attachment 379185 [details] WIP it begins
<rdar://problem/50850326>
Created attachment 379297 [details] WIP
Created attachment 379413 [details] WIP almost done. Just need to add stuff for customs we pull out of thin air (not in property storage and not in the static property table)
Created attachment 379431 [details] WIP
Comment on attachment 379431 [details] WIP Attachment 379431 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13062829 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases
Created attachment 379589 [details] patch
Comment on attachment 379589 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=379589&action=review > Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:394 > - return generateConditions( > + auto result = generateConditions( will turn this back into a return
Created attachment 379590 [details] patch
will add this as a test too: ``` <!DOCTYPE HTML> <html> <body> <div id="result"></div> <script> (function() { "use strict"; let setterCalled = false; function accessProperty() { let oScript = document.createElement("script"); oScript.src = Math.random(); } // Force "code optimization" by calling the function several times for (let i = 0; i < 1000; i++) { accessProperty(); } // Define a custom setter for HTMLScriptElement#src const descriptor = Object.getOwnPropertyDescriptor(HTMLScriptElement.prototype, "src"); Object.defineProperty(HTMLScriptElement.prototype, "src", { get: descriptor.get, set: function() { // Switch marker once setter is called setterCalled = true; descriptor.set.apply(this, arguments); }, enumerable: descriptor.enumerable, configurable: descriptor.configurable }); // Access the property once again, which should invoke the setter accessProperty(); // Print result document.getElementById("result").innerText = "Setter called: " + setterCalled; })(); </script> </body> </html> ```
Comment on attachment 379590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=379590&action=review > Source/JavaScriptCore/ChangeLog:1 > +2019-09-25 Saam Barati <sbarati@apple.com> I'll also note in the changelog that this is neutral on my microbenchmarks
Comment on attachment 379590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=379590&action=review Some comments for now while I continue reading the patch. > Source/JavaScriptCore/bytecode/PolyProtoAccessChain.cpp:67 > + if (!structure->propertyAccessesAreCacheable()) > + return nullptr; > + > + if (structure->isProxy()) > + return nullptr; > + Can you explain why this move from above is necessary? AFAICT, it looks like you just want to flatten dictionaries more aggressively. Is that correct? I suppose structure->propertyAccessesAreCacheable() will fail if the structure is for a dictionary. > Source/JavaScriptCore/runtime/ClassInfo.h:221 > unsigned staticClassSize; You probably missed this field because it only showed up after a rebase. Can you move it up with the rest as well? > Source/JavaScriptCore/runtime/Structure.h:56 > +struct HashTable; > +struct HashTableValue; Aren't we supposed to lump these with the struct list below? See "struct DumpContext" which starts the struct list below. > Source/JavaScriptCore/tools/JSDollarVM.cpp:647 > + StaticCustomAccessor* getter = new (NotNull, allocateCell<StaticCustomAccessor>(vm.heap)) StaticCustomAccessor(vm, structure); /getter/accessor/
Comment on attachment 379590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=379590&action=review >> Source/JavaScriptCore/bytecode/PolyProtoAccessChain.cpp:67 >> + > > Can you explain why this move from above is necessary? AFAICT, it looks like you just want to flatten dictionaries more aggressively. Is that correct? I suppose structure->propertyAccessesAreCacheable() will fail if the structure is for a dictionary. yes. Precisely, if it's an uncacheable dictionary, we will not flatten it. This is covered by a microbenchmark I added. >> Source/JavaScriptCore/runtime/ClassInfo.h:221 >> unsigned staticClassSize; > > You probably missed this field because it only showed up after a rebase. Can you move it up with the rest as well? good call. >> Source/JavaScriptCore/runtime/Structure.h:56 >> +struct HashTableValue; > > Aren't we supposed to lump these with the struct list below? See "struct DumpContext" which starts the struct list below. probably. Will fix >> Source/JavaScriptCore/tools/JSDollarVM.cpp:647 >> + StaticCustomAccessor* getter = new (NotNull, allocateCell<StaticCustomAccessor>(vm.heap)) StaticCustomAccessor(vm, structure); > > /getter/accessor/ will fix
Comment on attachment 379590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=379590&action=review r=me > Source/JavaScriptCore/bytecode/AccessCase.cpp:-313 > - return m_conditionSet.structuresEnsureValidityAssumingImpurePropertyWatchpoint(); Don't we need to check `if (!m_conditionSet.isValid())`? If we can ensure that this m_conditionSet is always valid, can we add assert instead? > Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:-65 > - return (numberOfConditionsWithKind(PropertyCondition::Presence) == 1) != (numberOfConditionsWithKind(PropertyCondition::Equivalence) == 1); I think implementing this inline here seems better than iterating three times. if (size() != 1) return false; const ObjectPropertyCondition& condition = *this.begin(); switch (condition.kind()) { case PropertyCondition::Presence: case PropertyCondition::Equivalence: case PropertyCondition::CustomFunctionEquivalence: return true; ... return false; } return false; > Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:62 > +class AdaptiveValueStructureStubClearingWatchpoint final : public AdaptiveInferredPropertyValueWatchpointBase { Personally, I think we should rename AdaptiveInferredPropertyValueWatchpointBase to something like `AdaptiveInferredPropertyValueWatchpointCollectionBase` or something because it is not a Watchpoint in fact (It is not inheriting Watchpoint). Can you add it as a FIXME comment? Or can you rename it? > Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:111 > + Bag<WTF::Variant<StructureTransitionStructureStubClearingWatchpoint, AdaptiveValueStructureStubClearingWatchpoint>> m_watchpoints; IIRC, this watchpoint class is allocated so many. So, we should save memory as much as possible. Do we have any ideas? If we do not have an idea for now, please put a FIXME about memory consumption. > Source/JavaScriptCore/jit/Repatch.cpp:552 > + vm, codeBlock, exec, structure, slot.base(), ident.impl(), 0); I think `static_cast<unsigned>(PropertyAttribute::None)` is better instead of `0`, it is ugly but anyway, it is more readable than 0. > Source/JavaScriptCore/runtime/ObjectPropertyChangeAdaptiveWatchpoint.h:32 > +template<typename WatchpointSet> Nice. At some point in the future, we should do this renaming for the other places too :P
Comment on attachment 379590 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=379590&action=review >> Source/JavaScriptCore/jit/Repatch.cpp:552 >> + vm, codeBlock, exec, structure, slot.base(), ident.impl(), 0); > > I think `static_cast<unsigned>(PropertyAttribute::None)` is better instead of `0`, it is ugly but anyway, it is more readable than 0. will fix
Created attachment 379862 [details] patch for landing
Comment on attachment 379862 [details] patch for landing Clearing flags on attachment: 379862 Committed r250540: <https://trac.webkit.org/changeset/250540>
All reviewed patches have been landed. Closing bug.
Regression: bug #202594