980532 - Prefetch sources on the client

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Right now we wait until the user clicks on a source to fetch the source text over the RDP. Instead we should greedily fetch the source and have it ready for when the user selects a new source.

Will have to be somewhat careful about not congesting the RDP transport with a ton of requests, so we should get source text one source at a time and with a delay between each request. Should also delay it after we get the initial sources so that we don't impact start up time.

Good bug for someone who knows the debugger frontend already; willing to mentor.

Comment 1

[Jayesh]

Hi Nick,

I am ready to work on this bug. As my first bug have just worked on the 'breadcrumbs' hover bug, and left out with the test cases; and have some familiarity with the debugger frontend.

Thanks,
Jayesh

Comment 2

[Jayesh]

Hi Nick,

I am ready to work on this bug. As my first bug have just worked on the 'breadcrumbs' hover bug, and left out with the test cases; and have some familiarity with the debugger frontend.

Thanks,
Jayesh

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Great!

Comment 4

[Jayesh]

Hi Nick,

Can you please guide me on this, like, from where to start... ??

Thanks,
Jayesh

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

In the _onNewSource callback[0] you can queue the new source for your source-text pre-fetching. Whenever you queue a new source, check to see if you are already fetching a source text. If you are, then just return. Otherwise take the first source from the queue. If this source already has source text in the cache[1], then take the next one and continue. When you find a source whose text hasn't been fetched yet, and fetch its source text[2]. Once you're done fetching the source text, it will be cached. If there aren't anymore sources in the queue, you're done. If there are, set a short timeout (50ms should be good) and repeat the whole process.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js?from=debugger-controller.js#1159
[1] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js?from=debugger-controller.js#1083
[2] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js?from=debugger-controller.js#1349

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hi Jayesh, are you making any progress? Need some help?

Comment 7

[Jayesh]

Hi Nick,

Give me some time. I am starting on it. As I get into any difficulty will let you know.

Thanks,
Jayesh

Comment 8

[Jayesh]

Hi Nick,

I started with debugging the code and did put breakpoint in debugger-controller.js. But I am really not getting what do you mean by "source" in "#comment 1". I am able to reach the breakpoint, but could not understand the case when I reach that point. Can you please put a light on that. 

Thanks,
Jayesh

Comment 9

[Jayesh]

Hi Nick,

Can you please throw more light on this. A small example will be more helpful...

Thanks,
Jayesh

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hey Jayesh, sorry I somehow missed your last couple comments. In the future check the "need for information from ..." checkbox below the comment area and add my email. Then I will be sure to respond in a timely manner.

A source is basically a JS file running on the page. It is represented by a SourceActor[0] on the backend and the frontend uses a SourceClient[1] to communicate over the Remote Debugging Protocol[2] with it (say to fetch the text content of the source).

[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2492
[1] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#2363
[2] https://wiki.mozilla.org/Remote_Debugging_Protocol#Loading_Script_Sources

The debugger's frontend wraps[3] the plain old SourceClient and provides caching of source text.

[3] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#1351

You can just call the above method early to preload a source's text and insert it into the frontend's cache. You can do this from the _onNewSource method[4] after a small delay (to avoid congesting the RDP transport).

[4] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#1145

Comment 11

[Jayesh]

Hey Nick,

Have started working on this. Will update you on difficulties.

Thanks,
Jayesh

Comment 12

[Jayesh]

Attachment: preload-1.diff

Comment 13

[Jayesh]

Hi Nick,

Please have a look at the diff file. I am re-using the constant "FETCH_SOURCE_RESPONSE_DELAY" which is already defined at the top of file.
I think we can reuse that instead of defining of a new one.

Thanks,
Jayesh

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8467272 [details] [diff] [review]
preload-1.diff

Review of attachment 8467272 [details] [diff] [review]:
-----------------------------------------------------------------

We shouldn't reuse FETCH_SOURCE_RESPONSE_DELAY because that is the amount of time to wait before we consider the fetching of a source as failed. Not quite the same thing. You should define your own constant (say `const PREFETCH_SOURCE_DELAY = 500; // ms`).

You should also attach an error handler to the promise returned from `getText` that just calls `DevToolsUtils.reportException`[0] so that we can debug failures to load the source.

Don't forget to run the debugger frontend tests with your changes:

  ./mach mochitest-devtools browser/devtools/debugger

[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/DevToolsUtils.js#48

::: browser/devtools/debugger/debugger-controller.js
@@ +1149,5 @@
>      }
>  
>      // Add the source in the debugger view sources container.
>      DebuggerView.Sources.addSource(aPacket.source, { staged: false });
> +    setTimeout(() => { this.getText(aPacket.source); }, FETCH_SOURCE_RESPONSE_DELAY);

Style nit: don't put this all on one line, try to conform to the existing style.

Comment 15

[Jayesh]

Attachment: preload-2.diff

Hi Nick,

Have attached a diff. Need a feedback.

Thanks,
Jayesh

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8469521 [details] [diff] [review]
preload-2.diff

Review of attachment 8469521 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'll play around with it and verify that loading sources does indeed feel snappier.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Switching sources does indeed feel snappier with this patch. Great!

Here is a try push: https://tbpl.mozilla.org/?tree=Try&rev=8af1cd1f84d0

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Accidental needinfo.

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

"ni? me" so I remember to check the try results so this can land...

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Jayesh, it looks like the following tests are failing on try, did you run the tests locally? Are they passing?

Perhaps the try push just got caught with some other bad commits that later got backed out, but I'd like a confirmation that the tests are passing locally for you before starting another try push.

browser/devtools/debugger/test/browser_dbg_pretty-print-11.js
https://tbpl.mozilla.org/php/getParsedLog.php?id=45994165&tree=Try

browser/devtools/debugger/test/browser_dbg_breakpoints-highlight.js
https://tbpl.mozilla.org/php/getParsedLog.php?id=45979199&tree=Try

Comment 21

[Jayesh]

Hi Nick,

Both the tests mentioned in comment 20 are working locally for me. Also, I forgot to mention you that I have "fx-team" repo in my local.

Thanks,
Jayesh

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok, new try push: https://tbpl.mozilla.org/?tree=Try&rev=962240ff0ecd

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Jayesh, it looks like the tests are still failing, see the try push in comment 22. You say they are passing locally, are they passing when you run the whole debugger mochitest suite (./mach mochitest-devtools browser/devtools/debugger) or were you just running the tests individually?

We can't land this until we get the tests pass :)

Comment 24

[Jayesh]

Hi Nick,

Can we get to know exactly which test case is failing...???

Thanks,
Jayesh

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jayesh from comment #24)
> Hi Nick,
> 
> Can we get to know exactly which test case is failing...???
> 
> Thanks,
> Jayesh

If you click on the failure in the try push, it will show you you the tests that failed and give you an option to open the full test log. See the bottom of this screen cap: https://i.imgur.com/95lGFaD.png

Comment 26

[Eddy Bruel [:ejpbruel]]

This looks like a dupe of 912600. Nick, can you please confirm?

Comment 27

[Eddy Bruel [:ejpbruel]]

(In reply to Eddy Bruel [:ejpbruel] from comment #26)
> This looks like a dupe of 912600. Nick, can you please confirm?

Nevermind that, that bug is about prefetching on the client.

Comment 28

[anjaly]

If its still open , I would like to work on the same :)

Comment 29

[Eddy Bruel [:ejpbruel]]

(In reply to anjaly from comment #28)
> If its still open , I would like to work on the same :)

Go for it! Nick should be able to mentor you.

Comment 30

[anjaly]

Attachment: debugger_exception.PNG

An exception is thrown when timeout is set in _onNewSource. (See Attachment)
Any pointers on this?

Comment 31

[Jason Laster [:jlast]]

I'm going to close this for two reasons:

1) the solution is focused on the old UI
2) I don't think we want to pre-fetch the source text.