Bug 49635 - Profiler implementation is fragile
Summary: Profiler implementation is fragile
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on: 49646
Blocks: 49809 49801
  Show dependency treegraph
 
Reported: 2010-11-16 17:43 PST by Gavin Barraclough
Modified: 2010-11-19 08:38 PST (History)
3 users (show)

See Also:


Attachments
the patch (49.05 KB, patch)
2010-11-16 17:57 PST, Gavin Barraclough
no flags Details | Formatted Diff | Diff
New improved implementation – breaks no tests, even fixes one! (37.10 KB, patch)
2010-11-18 15:55 PST, Gavin Barraclough
oliver: review-
Details | Formatted Diff | Diff
Ooops, cleaned up some unnecessary changes. (33.65 KB, patch)
2010-11-18 15:59 PST, Gavin Barraclough
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 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.
Comment 1 Gavin Barraclough 2010-11-16 17:57:18 PST
Created attachment 74073 [details]
the patch

Manually tested on sunspider, test case to check exception handling okay.
Comment 2 Gavin Barraclough 2010-11-16 19:03:52 PST
fixed in r72160
Comment 3 Ryosuke Niwa 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?
Comment 4 Csaba Osztrogonác 2010-11-17 03:12:05 PST
reopen because it was rolled out by http://trac.webkit.org/changeset/72176
Comment 5 Csaba Osztrogonác 2010-11-17 03:12:29 PST
Comment on attachment 74073 [details]
the patch

remove r+ from landed and rolled out patch
Comment 6 Gavin Barraclough 2010-11-18 15:55:10 PST
Created attachment 74308 [details]
New improved implementation – breaks no tests, even fixes one!
Comment 7 Oliver Hunt 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?
Comment 8 Gavin Barraclough 2010-11-18 15:59:27 PST
Created attachment 74310 [details]
Ooops, cleaned up some unnecessary changes.
Comment 9 Gavin Barraclough 2010-11-18 16:54:50 PST
fixed in r72351.
Comment 10 Andy Estes 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.
Comment 11 Andy Estes 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.