[webkit-reviews] review granted: [Bug 198023] [JSC] UnlinkedCodeBlock should be eventually jettisoned in VM mini mode : [Attachment 371624] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 198023: [JSC] UnlinkedCodeBlock should be eventually jettisoned in VM mini
mode
https://bugs.webkit.org/show_bug.cgi?id=198023

Attachment 371624: Patch

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




--- Comment #27 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 371624
  --> https://bugs.webkit.org/attachment.cgi?id=371624
Patch

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

Nice! r=me

> Source/JavaScriptCore/ChangeLog:54
> +	   UnlinkedCodeBlock link weak. We also use execution counter
information to reset this age: CodeBlock will reset undelying
UnlinkedCodeBlock's age if it has some
> +	   execution. While this heuristic is quite simple, it has some effect
in practice. Basically what happens with this heuristic is that
UnlinkedFunctionExecutable ->

"if it has some execution" -> "if it has executed"

> Source/JavaScriptCore/ChangeLog:67
> +	   memory footprint of our target application from about 6.5 MB to
about 5.9 MB.

nice!

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1377
> +	   if (!VM::isInMiniMode())

should call useUnlinkedCodeBlockJettisoning instead of miniMode.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1392
> +	   case JITType::DFGJIT:

Doesn't the DFG have an execution counter?

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:432
> +    unsigned m_age : 3;

nit: maybe  static_assert (1 << 3 >= maxAge) ?

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:151
> +    // We also keep builtin functions. The builtin functions are
foundamental library for JavaScript, which means that they can be used in
random places of
> +    // the client scripts. It is safe to assume that builtin functions are
used relatively frequently from many call sites.

Are we sure this is the case? Wouldn't this be captured by your executionCount
logic?

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:295
> +    if (VM::useUnlinkedCodeBlockJettisoning() && !m_isGeneratedFromCache &&
!isBuiltinFunction()) {

nit: You have this logic exactly in visitChildren too. Can you instead make it
a helper method on UnlinkedFunctionExecutable? Maybe "codeBlockEdgeMayBeWeak"
or "shouldConsiderCodeBlockEdgeAsWeak"