Bug 116753 - Remove Interpreter::retrieveLastCaller().
Summary: Remove Interpreter::retrieveLastCaller().
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-24 20:28 PDT by Mark Lam
Modified: 2013-05-25 13:23 PDT (History)
5 users (show)

See Also:


Attachments
the patch (16.12 KB, patch)
2013-05-24 21:15 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-05-24 20:28:53 PDT
Reduce the number of stack walking functions, and we're starting by removing Interpreter::retrieveLastCaller().
Comment 1 Mark Lam 2013-05-24 21:15:46 PDT
Created attachment 202873 [details]
the patch
Comment 2 Mark Lam 2013-05-24 21:44:20 PDT
I've run the riun-javascriptcode-tests and the layout tests for:
    fast/js
    fast/regex
    ietestcenter/JavaScript
    sputnik

No new failures were seen.  The remaining layout tests are still slowly cooking.
Comment 3 Geoffrey Garen 2013-05-24 22:54:36 PDT
Comment on attachment 202873 [details]
the patch

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

r=me

> Source/JavaScriptCore/API/JSContextRef.cpp:183
> +    for (Vector<StackFrame>::const_iterator iter = stackTrace.begin(); iter < stackTrace.end(); iter++) {

WebKit style prefers index over iterator in Vector iterations for a terse, easier-to-read code. (See http://www.webkit.org/coding/coding-style.html.)

> Source/JavaScriptCore/interpreter/Interpreter.h:250
> +        JS_EXPORT_PRIVATE void getStackTrace(Vector<StackFrame>& results, unsigned maxStackSize = INT_MAX);

Since this is an unsigned, it should be UINT_MAX.

> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:81
> +    exec->interpreter()->getStackTrace(stackTrace);

You can pass maxStackSize here, and remove the code to check it below.

> Source/WebCore/bindings/js/ScriptCallStackFactory.cpp:85
> +    Vector<StackFrame>::const_iterator iter = stackTrace.begin();
> +    iter++;
> +    for (; iter < stackTrace.end(); iter++) {
> +        frames.append(ScriptCallFrame(iter->friendlyFunctionName(exec), iter->friendlySourceURL(), iter->friendlyLineNumber()));

This case particularly benefits from index-style iteration, since index-style iteration doesn't need the dummy iter++ at the start.
Comment 4 Mark Lam 2013-05-25 13:23:21 PDT
Thanks for the review.  Addressed issues in Geoff's feedback.  Also fix 2 bugs in the patch:

1. In JSXMLHttpRequestCustom.cpp's JSXMLHttpRequest::send(), we need to explicitly specify size of the stackTrace Vector to guarantee that we have an entry at index 1.

2. In ScriptCallStackFactory.cpp's createScriptCallStack(), we need to increment maxStackSize by 1 before passing it to getStackTrace() since we're going to skip the 1st frame.

The updated patch has been tested with run-javascriptcore-tests and layout tests fast/js, fast/regex, ietestcenter/JavaScript, and sputnik.

Landed in r150690: <http://trac.webkit.org/changeset/150690>.