[webkit-reviews] review granted: [Bug 193546] [JSC] Reduce size of memory used for ShadowChicken : [Attachment 360424] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 193546: [JSC] Reduce size of memory used for ShadowChicken
https://bugs.webkit.org/show_bug.cgi?id=193546

Attachment 360424: Patch

https://bugs.webkit.org/attachment.cgi?id=360424&action=review




--- Comment #3 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 360424
  --> https://bugs.webkit.org/attachment.cgi?id=360424
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).