(In reply to Saam Barati from comment #0) > Keith found a bug. Basically, we null out the JSObject*'s prototype pointer > in the prototype map for poly proto today. This may cause one Executable to > end up with a Structure* meant for the create_this of a separate Executable. > This mean's it'll have the wrong InlineWatchpointSet&, causing it to > potentially pessimize another executable's allocation. My analysis is slightly wrong here. An executable will write over another executable's poly proto watchpoint if the prototypes happen to be the same.

(In reply to Saam Barati from comment #1) > (In reply to Saam Barati from comment #0) > > Keith found a bug. Basically, we null out the JSObject*'s prototype pointer > > in the prototype map for poly proto today. This may cause one Executable to > > end up with a Structure* meant for the create_this of a separate Executable. > > This mean's it'll have the wrong InlineWatchpointSet&, causing it to > > potentially pessimize another executable's allocation. > > My analysis is slightly wrong here. An executable will write over another > executable's poly proto watchpoint if the prototypes happen to be the same. And the other inputs also have to be the same to the prototype structure cache. So something like this I think: let p = {}; function foo() { class C { constructor() { this.x = 20; } }; C.prototype = p; } function bar() { class C { constructor() { this.x = 20; } }; C.prototype = p; } foo(); bar();

*** Bug 177952 has been marked as a duplicate of this bug. ***

Created attachment 322967 [details] WIP Waiting on experimental results for: https://bugs.webkit.org/show_bug.cgi?id=177987

Comment on attachment 322967 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=322967&action=review > Source/JavaScriptCore/heap/Heap.cpp:1414 > + vm()->prototypeMap.removeDeadEntries(); I don't think it's necessary. > Source/JavaScriptCore/runtime/PrototypeMap.cpp:40 > + PrototypeMapKey key { makePolyProtoStructure ? nullptr : prototype, inlineCapacity, classInfo, globalObject, executable }; Why can't you just make the first field be a prototype-or-executable field? > Source/JavaScriptCore/runtime/PrototypeMap.cpp:104 > +bool PrototypeMapKey::isDead() > +{ > + return (m_prototype && !Heap::isMarked(m_prototype)) > + || (m_globalObject && !Heap::isMarked(m_globalObject)) > + || (m_executable && !Heap::isMarked(m_executable)); > +} > + > +void PrototypeMap::removeDeadEntries() > +{ > + m_prototypes.removeIf([&] (JSObject* object) { > + return !Heap::isMarked(object); > + }); > + > + m_structures.removeIf([&] (auto& entry) { > + Structure* structure = entry.value; > + return !Heap::isMarked(structure) || entry.key.isDead(); > + }); I was wrong about us needing this. The lifetime of the structure takes care of everything for us, since the key is a bunch of raw pointers.

Comment on attachment 322967 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=322967&action=review >> Source/JavaScriptCore/runtime/PrototypeMap.cpp:104 >> + }); > > I was wrong about us needing this. The lifetime of the structure takes care of everything for us, since the key is a bunch of raw pointers. We don’t need it in ToT, however, I think we need it with this patch. The Structure won’t keep the Executable alive

Comment on attachment 322967 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=322967&action=review >> Source/JavaScriptCore/runtime/PrototypeMap.cpp:40 >> + PrototypeMapKey key { makePolyProtoStructure ? nullptr : prototype, inlineCapacity, classInfo, globalObject, executable }; > > Why can't you just make the first field be a prototype-or-executable field? Yeah we can do this.

Created attachment 323201 [details] WIP

Attachment 323201 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/StructureCache.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/VM.h:51: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 323205 [details] WIP

Attachment 323205 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/StructureCache.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/VM.h:51: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 323229 [details] WIP

Attachment 323229 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/StructureCache.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/VM.h:51: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 322967 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=322967&action=review >>> Source/JavaScriptCore/runtime/PrototypeMap.cpp:40 >>> + PrototypeMapKey key { makePolyProtoStructure ? nullptr : prototype, inlineCapacity, classInfo, globalObject, executable }; >> >> Why can't you just make the first field be a prototype-or-executable field? > > Yeah we can do this. No, we actually can't do this. This is the whole point of the patch. We must ensure that we get different structures before determining that we go poly proto. The chains of structures generating from the create_this belonging to the bytecode of an Executable must have different poly proto watchpoints.

Created attachment 323268 [details] patch

Attachment 323268 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/StructureCache.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/VM.h:51: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 323268 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=323268&action=review > Source/JavaScriptCore/runtime/StructureCache.h:44 > - explicit PrototypeMap(VM& vm) > + StructureCache(VM& vm) I'll add bad explicit here.

Created attachment 323283 [details] patch for landing

Attachment 323283 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/StructureCache.cpp:30: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 323283 [details] patch for landing Clearing flags on attachment: 323283 Committed r223125: <http://trac.webkit.org/changeset/223125>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34905866>