Bug 193546 – [JSC] Reduce size of memory used for ShadowChicken

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

1. Lazily allocate shadow chicken buffer
2. Reduce the size of packet anyway to reduce the buffer size even if it is allocated.

[Yusuke Suzuki]

Created attachment 360424 [details]
Patch

[Mark Lam]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r240637: <https://trac.webkit.org/changeset/240637>

[Radar WebKit Bug Importer]

<rdar://problem/47621712>

[Saam Barati]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 361495 [details]
WIP

[Saam Barati]

(In reply to Yusuke Suzuki from comment #10)
> Created attachment 361495 [details]
> WIP

What does this have to do with ShadowChicken?

[Yusuke Suzuki]

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

[Saam Barati]

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