Bug 200192 – [JSC] Make StructureChain less-tricky by using Auxiliary Buffer

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Make StructureChain less-tricky by using Auxiliary Buffer

[Yusuke Suzuki]

Created attachment 375015 [details]
Patch

[Yusuke Suzuki]

Created attachment 375016 [details]
Patch

[Yusuke Suzuki]

Created attachment 375019 [details]
Patch

[Yusuke Suzuki]

Created attachment 375024 [details]
Patch

[EWS Watchlist]

Comment on attachment 375024 [details]
Patch

Attachment 375024 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12821145

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases

[Saam Barati]

Comment on attachment 375024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375024&action=review

r=me

> Source/JavaScriptCore/runtime/StructureChain.cpp:48
> +    ++size; // Sentinel nullptr.

is it worth just having a size field? (Or we could also be terrible people and encode the size in the auxiliary pointer itself, since the size of a prototype chain is usually low. And if we exceed 16 bits we can OOM.)

> Source/JavaScriptCore/runtime/StructureChain.cpp:49
> +    WriteBarrier<Structure>* vector = static_cast<WriteBarrier<Structure>*>(vm.jsValueGigacageAuxiliarySpace.allocateNonVirtual(vm, (Checked<size_t>(size) * sizeof(WriteBarrier<Structure>)).unsafeGet(), nullptr, AllocationFailureMode::Assert));

What about OOM? Seems easy to just make this an allocation that can fail?

[Saam Barati]

(In reply to Build Bot from comment #5)
> Comment on attachment 375024 [details]
> Patch
> 
> Attachment 375024 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12821145
> 
> New failing tests:
> mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-
> validate-phases

Is this real?

[Yusuke Suzuki]

(In reply to Saam Barati from comment #7)
> (In reply to Build Bot from comment #5)
> > Comment on attachment 375024 [details]
> > Patch
> > 
> > Attachment 375024 [details] did not pass jsc-ews (mac):
> > Output: https://webkit-queues.webkit.org/results/12821145
> > 
> > New failing tests:
> > mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-
> > validate-phases
> 
> Is this real?

This test is known as flaky these days, and it is not a real issue with this patch.

[Yusuke Suzuki]

Comment on attachment 375024 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375024&action=review

Thanks!

>> Source/JavaScriptCore/runtime/StructureChain.cpp:48
>> +    ++size; // Sentinel nullptr.
> 
> is it worth just having a size field? (Or we could also be terrible people and encode the size in the auxiliary pointer itself, since the size of a prototype chain is usually low. And if we exceed 16 bits we can OOM.)

The current callers of StructureChain is now assuming that StructureChain allocation never fails. I think just crashing if OOM happens is OK here, as we did by allocating array with makeUniqueArray.
Since caching such a large StructureChain may not be profitable, it would be nice if we can make the caller allocation-failure-aware, and limit the size of StructureChain like <= UINT16_MAX.
I'll note it as FIXME with bugzilla link.

>> Source/JavaScriptCore/runtime/StructureChain.cpp:49
>> +    WriteBarrier<Structure>* vector = static_cast<WriteBarrier<Structure>*>(vm.jsValueGigacageAuxiliarySpace.allocateNonVirtual(vm, (Checked<size_t>(size) * sizeof(WriteBarrier<Structure>)).unsafeGet(), nullptr, AllocationFailureMode::Assert));
> 
> What about OOM? Seems easy to just make this an allocation that can fail?

I added the FIXME about allocation-failure-aware callers.

[Yusuke Suzuki]

Committed r248026: <https://trac.webkit.org/changeset/248026>

[Radar WebKit Bug Importer]

<rdar://problem/53738987>