[JSC] Make StructureChain less-tricky by using Auxiliary Buffer
Created attachment 375015 [details] Patch
Created attachment 375016 [details] Patch
Created attachment 375019 [details] Patch
Created attachment 375024 [details] Patch
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
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?
(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?
(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.
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.
Committed r248026: <https://trac.webkit.org/changeset/248026>
<rdar://problem/53738987>