Bug 177907 – The prototype cache should be aware of the Executable it generates a Structure for

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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.

[Saam Barati]

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

[Saam Barati]

(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();

[Saam Barati]

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

[Saam Barati]

Created attachment 322967 [details]
WIP

Waiting on experimental results for:
https://bugs.webkit.org/show_bug.cgi?id=177987

[Filip Pizlo]

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.

[Saam Barati]

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 323201 [details]
WIP

[Build Bot]

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.

[Saam Barati]

Created attachment 323205 [details]
WIP

[Build Bot]

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.

[Saam Barati]

Created attachment 323229 [details]
WIP

[Build Bot]

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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 323268 [details]
patch

[Build Bot]

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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 323283 [details]
patch for landing

[Build Bot]

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.

[WebKit Commit Bot]

Comment on attachment 323283 [details]
patch for landing

Clearing flags on attachment: 323283

Committed r223125: <http://trac.webkit.org/changeset/223125>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34905866>