[webkit-reviews] review granted: [Bug 144458] JIT call inline caches should cache calls to objects with getCallData/getConstructData traps : [Attachment 324527] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 144458: JIT call inline caches should cache calls to objects with
getCallData/getConstructData traps
https://bugs.webkit.org/show_bug.cgi?id=144458

Attachment 324527: Patch

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




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

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +	   Previously only JSFunction is handled by CallLinkInfo's caching
mechanism. This means that any

"that any" => "that"

> Source/JavaScriptCore/ChangeLog:12
> +	   2. CallLinkInfo tells nothing in the higher tier JITs.

"tells nothing" => "tells us nothing"?

> Source/JavaScriptCore/ChangeLog:19
> +	   for InternalFunction. Previously we do not record any information to
CallLinkInfo. Except for the

"we do not" => "we did not"

> Source/JavaScriptCore/ChangeLog:22
> +	   nodes for these InternalFunctions since CallLinkInfo tells nothing.
This patch teaches CallLinkInfo
> +	   handling InternalFunction too.

This last sentence is unneeded.

> Source/JavaScriptCore/ChangeLog:24
> +	   Attached microbenchmarks show performance improvement.

nice

> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:268
> +	      
handleSpecificCallee(static_cast<JSFunction*>(lastSeenCallee()));

Nit: Even though we're checking the type(), I think jsCast is the more
canonical cast and it also gives us even extra debug assertions.

> Source/JavaScriptCore/jit/JITOperations.cpp:944
> +	       ASSERT(!!codePtr);

nit: Might as well make it RELEASE_ASSERT since this check isn't performance
sensitive.

> Source/JavaScriptCore/jit/Repatch.cpp:956
> +	       // FIXME: We could add a fast path for InternalFunction with
closure call.

Please make a bug and link to it here or remove FIXME

> Source/JavaScriptCore/runtime/InternalFunction.cpp:95
> +    if (function->m_functionForConstruct == callHostFunctionAsConstructor)

Why use this as a flag instead of nullptr?