ShadowChicken's packet is large. It is 56 bytes. And we allocate 1000 packet storage for ShadowChicken. So ShadowChicken consumes 55K memory while VM is in action.
1. Lazily allocate shadow chicken buffer 2. Reduce the size of packet anyway to reduce the buffer size even if it is allocated.
Created attachment 360424 [details] Patch
Comment on attachment 360424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360424&action=review r=me with suggestions. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13472 > + RELEASE_ASSERT(vm().shadowChicken()); nit: I know this is not a perf critical function, but can we pre-compute and cache vm().shadowChicken() in a local variable above and use the local shadowChicken variable in the 3 places in this function. This more clearly communicates to the compiler that we're expecting the same shadowChicken value. It also matches the idiom in the rest of this patch of fetching shadowChicken first (for the null check cases). > Source/JavaScriptCore/jit/CCallHelpers.cpp:57 > + RELEASE_ASSERT(vm.shadowChicken()); Ditto, precache shadowChicken? > Source/JavaScriptCore/jit/JITOperations.cpp:2885 > + RELEASE_ASSERT(vm.shadowChicken()); Ditto. Precache shadowChicken? > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1894 > + RELEASE_ASSERT(vm.shadowChicken()); Ditto. Precache shadowChicken? > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1913 > + RELEASE_ASSERT(vm.shadowChicken()); Ditto. Precache shadowChicken? > Source/JavaScriptCore/tools/JSDollarVM.cpp:2065 > + vm->ensureShadowChicken(); Not that it matters that much because $vm is a debugging tool, but to be completely correct, I think this should be conditional on if (newDebuggerMode == DebuggerOn). In fact, we can even releaseShadowChicken if newDebuggerMode == DebuggerOff (though we've never done that before).
Comment on attachment 360424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360424&action=review Thanks! >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13472 >> + RELEASE_ASSERT(vm().shadowChicken()); > > nit: I know this is not a perf critical function, but can we pre-compute and cache vm().shadowChicken() in a local variable above and use the local shadowChicken variable in the 3 places in this function. This more clearly communicates to the compiler that we're expecting the same shadowChicken value. It also matches the idiom in the rest of this patch of fetching shadowChicken first (for the null check cases). Make sense. Fixed. >> Source/JavaScriptCore/jit/CCallHelpers.cpp:57 >> + RELEASE_ASSERT(vm.shadowChicken()); > > Ditto, precache shadowChicken? Fixed. >> Source/JavaScriptCore/jit/JITOperations.cpp:2885 >> + RELEASE_ASSERT(vm.shadowChicken()); > > Ditto. Precache shadowChicken? Fixed. >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1894 >> + RELEASE_ASSERT(vm.shadowChicken()); > > Ditto. Precache shadowChicken? Fixed. >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1913 >> + RELEASE_ASSERT(vm.shadowChicken()); > > Ditto. Precache shadowChicken? Fixed. >> Source/JavaScriptCore/tools/JSDollarVM.cpp:2065 >> + vm->ensureShadowChicken(); > > Not that it matters that much because $vm is a debugging tool, but to be completely correct, I think this should be conditional on if (newDebuggerMode == DebuggerOn). In fact, we can even releaseShadowChicken if newDebuggerMode == DebuggerOff (though we've never done that before). Nice, yeah. Fixed.
Committed r240637: <https://trac.webkit.org/changeset/240637>
<rdar://problem/47621712>
Comment on attachment 360424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360424&action=review > Source/JavaScriptCore/jit/JITExceptions.cpp:56 > + if (auto* shadowChicken = vm->shadowChicken()) Do we ever call setDebugger with JS frames on the stack?
Comment on attachment 360424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360424&action=review >> Source/JavaScriptCore/jit/JITExceptions.cpp:56 >> + if (auto* shadowChicken = vm->shadowChicken()) > > Do we ever call setDebugger with JS frames on the stack? I don't think so, but either is fine anyway since if there is no shadowChicken, tail-call log is not recorded.
Reopening to attach new patch.
Created attachment 361495 [details] WIP
(In reply to Yusuke Suzuki from comment #10) > Created attachment 361495 [details] > WIP What does this have to do with ShadowChicken?
(In reply to Saam Barati from comment #11) > (In reply to Yusuke Suzuki from comment #10) > > Created attachment 361495 [details] > > WIP > > What does this have to do with ShadowChicken? Oooooooops, I uploaded the patch to the wrong issue. The correct issue is https://bugs.webkit.org/show_bug.cgi?id=194375. Thank you for letting me know!
*** Bug 194431 has been marked as a duplicate of this bug. ***