Closed
Bug 1138975
Opened 10 years ago
Closed 10 years ago
Refactor breakpoint sliding for non-source mapped sources
Categories
(DevTools :: Debugger, defect)
Tracking
(firefox39 fixed)
RESOLVED
FIXED
Firefox 39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: ejpbruel, Assigned: ejpbruel)
Details
Attachments
(1 file)
(deleted),
patch
|
jlong
:
review+
|
Details | Diff | Splinter Review |
This patch refactors the breakpoint sliding algorithm for non-source mapped sources. I tried to heavily comment what I'm doing and why, so the patch should be fairly self-explanatory.
The end goal here is to get rid of setBreakpointForActorAtLocationSliding (which used to be setBreakpointForActor). Once this patch lands, the only thing depending on that function will be breakpoint sliding for source mapped sources, which I plan to tackle next.
Attachment #8571999 -
Flags: review?(jlong)
Assignee | ||
Comment 1•10 years ago
|
||
Note that this patch is based on top of the one in bug 1131646.
Comment 2•10 years ago
|
||
Comment on attachment 8571999 [details] [diff] [review]
patch
Review of attachment 8571999 [details] [diff] [review]:
-----------------------------------------------------------------
A few nits, but this looks sane. (edit: oops I never actually published this)
::: toolkit/devtools/server/actors/script.js
@@ +2844,5 @@
> + let lineToEntryPointsMap = [];
> +
> + // Iterate over all scripts that correspond to this source actor.
> + let scripts = this.scripts.getScriptsBySourceActor(this);
> + for (let script of scripts) {
What do you think about putting this into its own helper function, like _scriptEntryPointsByLine?
Also, it seems like we could cache this, right? Not sure setting a breakpoint really needs to be that optimized though. Although it might speed up refreshing a page with breakpoint. Don't worry about caching for now.
@@ +2952,5 @@
> + this,
> + generatedLine
> + ).filter((script) => !actor.hasScript(script));
> +
> + // Find all entry points that correspond to the given location.
Seems like it'd be nice to make a few different "findEntryPoints" helper methods that would discover entry points in a few different ways. It would make it a little easier to read through the algorithm, and provide opportunities for caching later on.
@@ +3137,5 @@
> }
> actor.addScript(script, this.threadActor);
> }
>
> + return;
What's the point of this return?
Attachment #8571999 -
Flags: review?(jlong) → review+
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to James Long (:jlongster) from comment #2)
> Comment on attachment 8571999 [details] [diff] [review]
> patch
>
> Review of attachment 8571999 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> A few nits, but this looks sane. (edit: oops I never actually published this)
>
> ::: toolkit/devtools/server/actors/script.js
> @@ +2844,5 @@
> > + let lineToEntryPointsMap = [];
> > +
> > + // Iterate over all scripts that correspond to this source actor.
> > + let scripts = this.scripts.getScriptsBySourceActor(this);
> > + for (let script of scripts) {
>
> What do you think about putting this into its own helper function, like
> _scriptEntryPointsByLine?
>
It's not yet clear to me to what extent breakpoint code can/should be refactored. For that reason, I'd like to hold off from abstracting until the factor is complete.
> Also, it seems like we could cache this, right? Not sure setting a
> breakpoint really needs to be that optimized though. Although it might speed
> up refreshing a page with breakpoint. Don't worry about caching for now.
>
I doubt that would be worthwhile. Since setting a breakpoint is something that involves a user interaction 99% of the time, as long as the total interaction takes < 30ms to complete, the user most likely wouldn't even notice further improvements.
> @@ +2952,5 @@
> > + this,
> > + generatedLine
> > + ).filter((script) => !actor.hasScript(script));
> > +
> > + // Find all entry points that correspond to the given location.
>
> Seems like it'd be nice to make a few different "findEntryPoints" helper
> methods that would discover entry points in a few different ways. It would
> make it a little easier to read through the algorithm, and provide
> opportunities for caching later on.
>
I will probably end up doing something like that, but see above.
> @@ +3137,5 @@
> > }
> > actor.addScript(script, this.threadActor);
> > }
> >
> > + return;
>
> What's the point of this return?
Oops :-)
Assignee | ||
Comment 4•10 years ago
|
||
Try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=999def0d4aa9
Assignee | ||
Comment 5•10 years ago
|
||
I'm seeing a test failure in browser_dbg_source-maps-04.js that I cannot reproduce locally. What's strange is that the test seems to pass on all Linux builds except Linux debug. What's more, the nature of the bug (setting a breakpoint returns an error) is such that I'd expect it to fail on all platforms, or not at all.
I've retriggered the offending test suite. If I cannot reproduce it, I'm going to assume that this is an intermittent test failure due to a subtle race condition that was either caused by my changes, or simply made visible, and will file a followup bug for that.
Assignee | ||
Comment 6•10 years ago
|
||
Ok, so the failure is not intermittent, but consistent. Investigating.
Assignee | ||
Comment 7•10 years ago
|
||
The test failures were most likely caused by a patch that landed before this one, and should now be resolved.
New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbdca3003850
Assignee | ||
Comment 8•10 years ago
|
||
New try push has some failures on Windows, but those look like infra issues. Looks fine otherwise, so pushing to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/de24b63c6966
Comment 9•10 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2336344&repo=fx-team
Flags: needinfo?(ejpbruel)
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #9)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=2336344&repo=fx-team
That's strange, since those failures didn't show up on try. Are you sure this isn't just an intermittent?
In any case, I've made a new try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=540316168441
Flags: needinfo?(ejpbruel) → needinfo?(cbook)
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #10)
> (In reply to Carsten Book [:Tomcat] from comment #9)
> > sorry had to back this out for test failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=2336344&repo=fx-team
>
> That's strange, since those failures didn't show up on try. Are you sure
> this isn't just an intermittent?
>
> In any case, I've made a new try push for this patch:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=540316168441
After studying the test for a bit, I think the test is at fault here, because it is gc sensitive. That would explain why it only fails on Linux debug, and only some of the time. I'm going to refactor that test as part of the patch, then try to land again.
Flags: needinfo?(cbook)
Assignee | ||
Comment 12•10 years ago
|
||
New try push with browser_dbg_source-maps-04.js refactored. Hopefully, it should now no longer fail on Linux:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=087e6453c45d
Assignee | ||
Comment 13•10 years ago
|
||
Nope, that did not quite work. Cancelled that try run and started a new one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=750bb16ee6c5
Assignee | ||
Comment 14•10 years ago
|
||
Try push looks good, pushing to fx-team again:
https://hg.mozilla.org/integration/fx-team/rev/844a5d08948f
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•