Closed Bug 1413605 Opened 7 years ago Closed 7 years ago

MarkupContainer creates event listeners on window for each node in the Inspector

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: adrian17, Assigned: adrian17)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1413605 - Reuse the parent markup tree's listeners instead of making a listener per container. r=jdescottes
4.53 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
http://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/views/markup-container.js#66-67 These listeners are created for every node in the inspector, but they do basically the same thing (if an item is dragged, move it or drop it) and probably could be replaced with a single listener (that knows about the currently dragged item). While working in the inspector on more complex pages, the number of nodes (and thus, window listeners) opened in the inspector can reach hundreds and even thousands. It can be seen on profiles when: - using "expand all" that results in a lot of expanded nodes (as time of addEventListener scales with number of existing listeners attached to the node): https://perfht.ml/2iSejts - moving the mouse in the inspector while hundreds of nodes are expanded: https://perfht.ml/2z5yHvr
Attached a patch that fixes the issue. Technically I'd call it a proof of concept, as I'm not yet comfortable with the codebase and I'm not 100% sure this is the way to go. However, all tests pass and the results are good. With the patch, for a basic document with 3000 <div>s (nested in 3 layers, to avoid the nodes being hidden): - time of "Expand all" on <body> improved from 3.2s to 2.1s - after expanding the nodes, when moving the cursor over the inspector, the highlighting of item rows is much more fluid.
Attachment #8924615 - Flags: review?(jdescottes)
Assignee: nobody → adrian.wielgosik
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Comment on attachment 8924615 [details] [diff] [review] Reuse the parent markup tree's listeners instead of making a listener per container. Review of attachment 8924615 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Adrian! Looks like a really nice win. I think the change makes sense and should be safe. Needs a bit more comments and to change the name of the property on markup.js, but otherwise this looks good! Can you reupload a patch addressing my comments and using a proper commit message (Bug 1234567 - summary. r=jdescottes) I pushed your changeset to our continuous integration platform: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51e8a4781bfcc15635b837b62e2cffa905b9a2d8 I will also create a follow up to add a performance test for this change. ::: devtools/client/inspector/markup/markup.js @@ +198,4 @@ > }, > > isDragging: false, > + selectedContainer: null, We already have a "_selectedContainer" in markup.js so this naming will be confusing. Let's rename the new one to "_draggedContainer" (_ for consistency, and this is really about the item being dragged, not selected) ::: devtools/client/inspector/markup/views/markup-container.js @@ +526,5 @@ > > /** > * On mouse up, stop dragging. > */ > + onMouseUp: Task.async(function* () { Let's update the comment to explain that this method is now supposed to be called by the markup.js container. @@ +549,4 @@ > /** > * On mouse move, move the dragged element and indicate the drop target. > */ > + onMouseMove: function (event) { idem, update the comment @@ +720,3 @@ > this.elt.removeEventListener("mousedown", this._onMouseDown); > this.elt.removeEventListener("dblclick", this._onToggle); > this.tagLine.removeEventListener("keydown", this._onKeyDown, true); I guess we should attempt to clear this.markup.selectedContainer in destroy now instead? if (this.markup.selectedContainer == this) { this.markup.selectedContainer = null; }
Attachment #8924615 - Flags: review?(jdescottes) → feedback+
Updated patch, fixed test failures. `_draggedContainer` is also potentially slightly confusing, because in the pre-drag stage we can have a non-null `_draggedContainer` while `isDragging === false`. But other than that, it's definitely a much better name.
Attachment #8924615 - Attachment is obsolete: true
Attachment #8924956 - Flags: review?(jdescottes)
Fixed copy-paste mistake in commit name.
Attachment #8924956 - Attachment is obsolete: true
Attachment #8924956 - Flags: review?(jdescottes)
Attachment #8924958 - Flags: review?(jdescottes)
Comment on attachment 8924958 [details] [diff] [review] Bug 1413605 - Reuse the parent markup tree's listeners instead of making a listener per container. r=jdescottes Review of attachment 8924958 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks for addressing my comments. https://treeherder.mozilla.org/#/jobs?repo=try&revision=10914c7c322cca738fe6590900601f2e159b479e Let's wait for this new try push and if tests are green, we will land your patch to an integration branch. Normally your patch should make it in 58. Thanks a lot for fixing this out of the blue!
Attachment #8924958 - Flags: review?(jdescottes) → review+
See Also: → 1414286
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/23c2f77d9377 Reuse the parent markup tree's listeners instead of making a listener per container. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/23c2f77d9377
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: