Bug 173698 - Web Inspector: Pausing with a deep call stack can be very slow, avoid eagerly generating object previews
Summary: Web Inspector: Pausing with a deep call stack can be very slow, avoid eagerly...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-22 00:23 PDT by Joseph Pecoraro
Modified: 2017-06-22 14:12 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (32.26 KB, patch)
2017-06-22 00:39 PDT, Joseph Pecoraro
mattbaker: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-06-22 00:23:12 PDT
Pausing with a deep call stack can be very slow, avoid eagerly generating object previews.

Test:
<script>
var count = 0;
function f() {
    if (++count > 1000)
        throw new Error;
    f();
}
f();
</script>

Steps to Reproduce:
1. Inspect attached test page
2. Enable Pause on Uncaught Exceptions
3. Reload the page (triggers the exception)
  => Pause, takes a very long time to show debugger UI

Notes:
We spend about 95% of the time in the backend generating object previews for each of the 1000 CallFrame's `thisObject`. Which is the `window` in this example. We can avoid generating the ObjectPreview eagerly (here), and instead generate them lazily if we ever need to see them in the frontend.
Comment 1 Joseph Pecoraro 2017-06-22 00:39:21 PDT
Created attachment 313593 [details]
[PATCH] Proposed Fix
Comment 2 Build Bot 2017-06-22 00:41:31 PDT
Attachment 313593 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.h:65:  The parameter name "preview" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Matt Baker 2017-06-22 10:05:47 PDT
Comment on attachment 313593 [details]
[PATCH] Proposed Fix

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

> LayoutTests/ChangeLog:10
> +        Test the new protocol commnand `Runtime.getPreview` as well as the frontend

commnand -> command

> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:12
> +    

Remove whitespace.
Comment 4 Matt Baker 2017-06-22 11:22:39 PDT
Comment on attachment 313593 [details]
[PATCH] Proposed Fix

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

I've gotten through the tests, will continue with backend/frontend.

> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:51
> +            if (callFrame.thisObject.canLoadPreview()) {

Should we also assert that callFrame.thisObject.canLoadPreview()? Later it's assumed that every call frame was mapped to a Promise, and access callFrame.thisObject.preview for every frame.

> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:70
> +                InspectorTest.expectThat(expectedFrame.thisValue[0] === prop.name, `'this' value should have expected property: ${expectedFrame.thisValue[0]}`);

Could use InspectorTest.expectEqual here.

> LayoutTests/inspector/runtime/getPreview-expected.txt:28
> +PASS: RemoteObject.updatePreview should return null for a object RemoteObject.

This last result seemed strange at first, until reading the test code and seeing that it's the fake remote object.

> LayoutTests/inspector/runtime/getPreview-expected.txt:46
> +Error: Could not find InjectedScript for objectId

Nit: include a period at end of sentence, to match the lines above.

> LayoutTests/inspector/runtime/getPreview.html:8
> +    InspectorTest.debug();

Remove InspectorTest.debug();

> LayoutTests/inspector/runtime/getPreview.html:60
> +            function testRemoteObject(remoteObject, type) {

`type` isn't used. Was it meant to be included in test output instead of ${remoteObject.type}?
Comment 5 Joseph Pecoraro 2017-06-22 11:28:07 PDT
Comment on attachment 313593 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:51
>> +            if (callFrame.thisObject.canLoadPreview()) {
> 
> Should we also assert that callFrame.thisObject.canLoadPreview()? Later it's assumed that every call frame was mapped to a Promise, and access callFrame.thisObject.preview for every frame.

Nope. The `thisObject` of a CallFrame could be null, or a primitive value, and I believe that is the case in this test.

I'm not sure what you mean by call frame's being mapped to promises. Here we are calling `updatePreview` only on a few call frames, and wait for all of those requests to finish (having updated the RemoteObjects in place) before proceeding to loop over all call frames.

>> LayoutTests/inspector/runtime/getPreview-expected.txt:28
>> +PASS: RemoteObject.updatePreview should return null for a object RemoteObject.
> 
> This last result seemed strange at first, until reading the test code and seeing that it's the fake remote object.

Ahh yea, I'll use that extra `type` argument to clarify this case!

>> LayoutTests/inspector/runtime/getPreview-expected.txt:46
>> +Error: Could not find InjectedScript for objectId
> 
> Nit: include a period at end of sentence, to match the lines above.

This is just logging the exact protocol `error` value.
Comment 6 Matt Baker 2017-06-22 11:41:03 PDT
Comment on attachment 313593 [details]
[PATCH] Proposed Fix

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

Nice work, r=me with a few nits/comments.

> Source/JavaScriptCore/ChangeLog:10
> +        the `thisObject` of each of the call frame's. In some cases, this could be more

No apostrophe in frames.

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:240
> +        var object = this._objectForId(parsedObjectId);

Nit: The rest of the file still uses var, but why not use let for new code?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:475
> +        var result = evalFunction.call(object, expression, commandLineAPI);

Nit: let

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:170
> +    muteConsole();

This is such a common pattern in InspectorRuntimeAgent, I wonder if it would be worth making into a RAII-style guard.
Comment 7 Joseph Pecoraro 2017-06-22 14:12:21 PDT
<https://trac.webkit.org/changeset/218718/webkit>