loading a source should not block on breakpoint positions
Categories
(DevTools :: Debugger, enhancement, P2)
Tracking
(firefox68 affected)
| Tracking | Status | |
|---|---|---|
| firefox68 | --- | affected |
People
(Reporter: jlast, Assigned: jlast)
References
Details
Attachments
(1 file)
Currently we wait for breakpoint positions to be available before we consider a file loaded. This is partially responsible for the recent performance regressions
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 1•5 years ago
|
||
Pushed by jlaster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b5318d1a3e75 loading a source should not block on breakpoint positions. r=loganfsmyth
|
||
Comment 3•5 years ago
|
||
| bugherder | ||
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 4•5 years ago
•
|
||
This likely triggered a 70% pause performance regression.
|
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 5•5 years ago
|
||
|
||
Comment 6•5 years ago
|
||
Oh yeah, I ran these yesterday but I haven't had time to figure out how to compare them properly with the dashboard output.
Central with patch reverted:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44645c108b7c45e7c2899a30148cec458fc80d40
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=44645c108b7c45e7c2899a30148cec458fc80d40
Central with patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eb418f46c420902a07a85c9a04d7d29477ee956
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=1eb418f46c420902a07a85c9a04d7d29477ee956
Central with patch and getBreakpointPositions optimization (https://bugzilla.mozilla.org/show_bug.cgi?id=1536201):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2707d2b5cd13b1922cacbbe4f9475c43f699e8a4
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=2707d2b5cd13b1922cacbbe4f9475c43f699e8a4
|
|
||
Comment 7•5 years ago
|
||
I'm not sure what to do here. I think this may just be the new normal, depending on what we want. The DAMP tests at the moment are mostly one long series of operations, so changes can shift the time spent in one place to another. The question in this case is what we're trying to measure int he first place.
This patch specifically:
- Stops waiting for breakpoint location information when we load file content
- Starts waiting for it when you add a breakpoint
I'm thinking what is happening here is that we're basically moving this time from other tests and concentrating it in the pause test. Because loading the page content is no longer waiting, it is faster, but then the test continues and the first thing we try to do is add a breakpoint. Since nothing before this point is waiting for the breakpoints to load, that now ends up happening as part of the pause time.
So the question is, what is pause actually trying to time? We could easily for instance add an explicit wait for the breakpoint metadata before running the pause test. That would likely put the timing right back where it was.
|
|
||
Comment 8•5 years ago
•
|
||
On my local machine I'm seeing timings like this:
| Pre Patch | Post Patch | Post Patch + await | |
|---|---|---|---|
| open.DAMP | 1309.994118 | 1068.523077 | 1257.133333 |
| reload.DAMP | 1185.523529 | 994.6846154 | 1222.208333 |
| pause.DAMP | 286.8647059 | 453.7076923 | 274.4666667 |
| stepIn.DAMP | 626.0058824 | 604.1846154 | 639.8416667 |
| stepOver.DAMP | 410.7 | 386.3692308 | 378 |
| stepOut.DAMP | 494.9117647 | 513.1769231 | 477.3666667 |
| close.DAMP | 92.79411765 | 94.5 | 94.875 |
where "Patch + await" basically just re-adds the await keyword to wait for the breakpoint info to load when loading file text. You can see how that immediately shifts the time back from pause and into open/reload.
|
|
Assignee | |
Comment 9•5 years ago
|
||
Thanks for the analysis Logan, I agree that this is definitely a question of how we're measuring.
I think it is okay to treat this as the new normal in part because we're also measuring stepping in the same file.
In the future, we can also add some new tests in the future that isolate some of this interaction. e.g
- adding a second breakpoint in the same source
- pausing in a new file w/o a breakpoint ...
Description
•