Bug 201994

Summary:

Inline caching is wrong for custom accessors and custom values

Product:

WebKit

Reporter:

Saam Barati <saam>

Component:

JavaScriptCore

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, mcatanzaro, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=202827

Bug Depends on:

202594

Bug Blocks:

Attachments:

Description	Flags
WIP	none
WIP	none
WIP	none
WIP	ews-watchlist: commit-queue-
patch	none
patch	ysuzuki: review+
patch for landing	none

Saam Barati

Reported 2019-09-19 11:25:34 PDT

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

Attachments
WIP (23.00 KB, patch) 2019-09-19 18:55 PDT, Saam Barati	no flags	Details Formatted Diff Diff
WIP (30.60 KB, patch) 2019-09-20 17:56 PDT, Saam Barati	no flags	Details Formatted Diff Diff
WIP (42.63 KB, patch) 2019-09-23 18:26 PDT, Saam Barati	no flags	Details Formatted Diff Diff
WIP (43.28 KB, patch) 2019-09-23 22:47 PDT, Saam Barati	ews-watchlist: commit-queue-	Details Formatted Diff Diff
patch (55.55 KB, patch) 2019-09-25 16:35 PDT, Saam Barati	no flags	Details Formatted Diff Diff
patch (55.38 KB, patch) 2019-09-25 16:40 PDT, Saam Barati	ysuzuki: review+	Details Formatted Diff Diff
patch for landing (59.92 KB, patch) 2019-09-30 17:03 PDT, Saam Barati	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Saam Barati

Comment 1 2019-09-19 14:21:40 PDT

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

Saam Barati

Comment 2 2019-09-19 18:55:22 PDT

Created attachment 379185 [details] WIP it begins

Saam Barati

Comment 3 2019-09-20 17:53:28 PDT

<rdar://problem/50850326>

Saam Barati

Comment 4 2019-09-20 17:56:11 PDT

Created attachment 379297 [details] WIP

Saam Barati

Comment 5 2019-09-23 18:26:44 PDT

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)

Saam Barati

Comment 6 2019-09-23 22:47:19 PDT

Created attachment 379431 [details] WIP

EWS Watchlist

Comment 7 2019-09-24 01:04:16 PDT

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

Saam Barati

Comment 8 2019-09-25 16:35:38 PDT

Created attachment 379589 [details] patch

Saam Barati

Comment 9 2019-09-25 16:36:39 PDT

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

Saam Barati

Comment 10 2019-09-25 16:40:00 PDT

Created attachment 379590 [details] patch

Saam Barati

Comment 11 2019-09-26 08:11:51 PDT

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

Saam Barati

Comment 12 2019-09-26 08:46:38 PDT

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

Mark Lam

Comment 13 2019-09-30 10:37:24 PDT

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/

Saam Barati

Comment 14 2019-09-30 11:15:19 PDT

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

Yusuke Suzuki

Comment 15 2019-09-30 12:28:39 PDT

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

Saam Barati

Comment 16 2019-09-30 16:19:00 PDT

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

Saam Barati

Comment 17 2019-09-30 17:03:43 PDT

Created attachment 379862 [details] patch for landing

WebKit Commit Bot

Comment 18 2019-09-30 17:50:53 PDT

Comment on attachment 379862 [details] patch for landing Clearing flags on attachment: 379862 Committed r250540: <https://trac.webkit.org/changeset/250540>

WebKit Commit Bot

Comment 19 2019-09-30 17:50:55 PDT

All reviewed patches have been landed. Closing bug.

Michael Catanzaro

Comment 20 2019-10-04 16:36:32 PDT

Regression: bug #202594

Note You need to log in before you can comment on or make changes to this bug.