RESOLVED FIXED 49635
Profiler implementation is fragile
https://bugs.webkit.org/show_bug.cgi?id=49635
Summary Profiler implementation is fragile
Gavin Barraclough
Reported 2010-11-16 17:43:49 PST
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.
Attachments
the patch (49.05 KB, patch)
2010-11-16 17:57 PST, Gavin Barraclough
no flags
New improved implementation – breaks no tests, even fixes one! (37.10 KB, patch)
2010-11-18 15:55 PST, Gavin Barraclough
oliver: review-
Ooops, cleaned up some unnecessary changes. (33.65 KB, patch)
2010-11-18 15:59 PST, Gavin Barraclough
oliver: review+
Gavin Barraclough
Comment 1 2010-11-16 17:57:18 PST
Created attachment 74073 [details] the patch Manually tested on sunspider, test case to check exception handling okay.
Gavin Barraclough
Comment 2 2010-11-16 19:03:52 PST
fixed in r72160
Ryosuke Niwa
Comment 3 2010-11-16 21:36:27 PST
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
Comment 4 2010-11-17 03:12:05 PST
reopen because it was rolled out by http://trac.webkit.org/changeset/72176
Csaba Osztrogonác
Comment 5 2010-11-17 03:12:29 PST
Comment on attachment 74073 [details] the patch remove r+ from landed and rolled out patch
Gavin Barraclough
Comment 6 2010-11-18 15:55:10 PST
Created attachment 74308 [details] New improved implementation – breaks no tests, even fixes one!
Oliver Hunt
Comment 7 2010-11-18 15:57:05 PST
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
Comment 8 2010-11-18 15:59:27 PST
Created attachment 74310 [details] Ooops, cleaned up some unnecessary changes.
Gavin Barraclough
Comment 9 2010-11-18 16:54:50 PST
fixed in r72351.
Andy Estes
Comment 10 2010-11-19 08:33:57 PST
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
Comment 11 2010-11-19 08:38:25 PST
fast/profiler/throw-exception-from-eval.html also fails on Leopard. See https://bugs.webkit.org/show_bug.cgi?id=49801.
Note You need to log in before you can comment on or make changes to this bug.