Bug 127910 - Avoid eagerly creating the JSActivation when the debugger is attached
Summary: Avoid eagerly creating the JSActivation when the debugger is attached
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: 128024
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-30 08:40 PST by Mark Lam
Modified: 2014-01-31 17:21 PST (History)
7 users (show)

See Also:


Attachments
work in progress: still need to analyze a few test failures. (36.19 KB, patch)
2014-01-30 11:17 PST, Mark Lam
no flags Details | Formatted Diff | Diff
the patch. (40.22 KB, patch)
2014-01-31 16:39 PST, Mark Lam
oliver: 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 2014-01-30 08:40:42 PST
This results in about a 2x speed up on Octane.  Patch coming.
Comment 1 Mark Lam 2014-01-30 11:17:29 PST
Created attachment 222693 [details]
work in progress: still need to analyze a few test failures.
Comment 2 Mark Lam 2014-01-31 16:39:15 PST
Created attachment 222864 [details]
the patch.

This patch has passed all the layout tests with and without debugger bytecode generated.  I've also done a smoke test of using the WebInspector to step through some code.

Octane scores for this patch:
            baseline w/o WebInspector: 11621
            patched  w/o WebInspector: 11801
            baseline w/ WebInspector:  3295
            patched  w/ WebInspector:  7070   2.1x improvement
Comment 3 Oliver Hunt 2014-01-31 16:42:29 PST
Comment on attachment 222864 [details]
the patch.

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

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:119
> +        JSActivation* activation;
> +        activation = JSActivation::create(*codeBlock->vm(), m_callFrame, codeBlock);

one line

> Source/JavaScriptCore/interpreter/CallFrameInlines.h:145
> +    ASSERT(codeBlock->needsActivation());

i'm paranoid make this a RELEASE_ASSERT as it doesn't look like it should be hot and the activation register call will mean you're going to have decent memory locality
Comment 4 Geoffrey Garen 2014-01-31 16:49:26 PST
Comment on attachment 222864 [details]
the patch.

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

> Source/JavaScriptCore/runtime/JSScope.h:66
> +inline const char* resolveModeName(ResolveMode mode)
> +{
> +    static const char* const names[] = {
> +        "ThrowIfNotFound",
> +        "DoNotThrowIfNotFound"
> +    };
> +    return names[mode];
> +}
> +
> +inline const char* resolveTypeName(ResolveType type)

I think that inlining these will cause copies of your tables in places. Let's put these into the .cpp file.
Comment 5 Mark Lam 2014-01-31 17:21:02 PST
Thanks for the reviews.  Suggested feedback has been applied.  Landed in r163223: <http://trac.webkit.org/r163223>.