Bug 49635 – Profiler implementation is fragile

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

[Gavin Barraclough]

The profile presently requires the exception handling mechanism to explicitly remove all stack frames that are exited during the exception unwind mechanism.  This is fragile in a number of ways:
 * We have to change bytecode register allocation when compiling code to run when profiling, to preserve the callee function (this is also required to call did_call after the call has returned).
 * In the JIT we have to maintain additional data structures (CodeBlock::RareData::m_functionRegisterInfos) to map back to the register containing the callee.
 * In the interpreter we use 'magic values' to offset into the instruction stream to rediscover the register containing the function.

Instead, move profiling into the head and tail of functions.
 * This correctly accounts the cost of the call itself to the caller.
 * This allows us to access the callee function object from the callframe.
 * This means that at the point a call is made we can track the stack depth on the ProfileNode.
 * When unwinding we can simply report the depth at which the exception is being handled - all call frames above this level are freed.

[Gavin Barraclough]

Created attachment 74073 [details]
the patch

Manually tested on sunspider, test case to check exception handling okay.

[Gavin Barraclough]

fixed in r72160

[Ryosuke Niwa]

This change seems to be responsible for a whole bunch of SL test failures:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r72164%20(20945)/results.html

Could you look into this?

[Csaba Osztrogonác]

reopen because it was rolled out by http://trac.webkit.org/changeset/72176

[Csaba Osztrogonác]

Comment on attachment 74073 [details]
the patch

remove r+ from landed and rolled out patch

[Gavin Barraclough]

Created attachment 74308 [details]
New improved implementation – breaks no tests, even fixes one!

[Oliver Hunt]

Comment on attachment 74308 [details]
New improved implementation – breaks no tests, even fixes one!

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

> JavaScriptCore/interpreter/Interpreter.cpp:566
> +//    if (Profiler* profiler = *Profiler::enabledProfilerReference()) {
> +//        if (callFrame->callee())
> +//            profiler->didExecute(callFrame, callFrame->callee());
> +//        else
> +//            profiler->didExecute(callFrame, codeBlock->ownerExecutable()->sourceURL(), codeBlock->ownerExecutable()->lineNo());
> +//    }

really?

[Gavin Barraclough]

Created attachment 74310 [details]
Ooops, cleaned up some unnecessary changes.

[Gavin Barraclough]

fixed in r72351.

[Andy Estes]

Looks like 'plugins/refcount-leaks.html' is failing on Leopard due to this change. Filed https://bugs.webkit.org/show_bug.cgi?id=49809.

[Andy Estes]

fast/profiler/throw-exception-from-eval.html also fails on Leopard. See https://bugs.webkit.org/show_bug.cgi?id=49801.