Closed
Bug 892532
Opened 11 years ago
Closed 11 years ago
Add an optional fast-path to CustomizableUI.isWidgetRemovable
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 28
People
(Reporter: jaws, Assigned: jaws)
References
Details
(Whiteboard: [Australis:M8])
Attachments
(1 file, 2 obsolete files)
4.27 KB,
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
In my local timings I'm seeing about 4-7ms during window open coming from us looking for nodes within isWidgetRemovable when the calling function already had a reference to the node. Try pushes coming, but locally this patch shows a savings of 4-7ms (some window openings are 4ms faster, others are 7ms faster).
Attachment #774013 -
Flags: review?(mnoorenberghe+bmo)
Assignee | ||
Comment 1•11 years ago
|
||
Baseline try push, without patch applied: https://tbpl.mozilla.org/?tree=Try&rev=d0e5e0d84f47 With patch applied: https://tbpl.mozilla.org/?tree=Try&rev=168cb53a983b
Assignee | ||
Comment 2•11 years ago
|
||
Hm, what do you think about the Talos numbers? Should we take it? It is undoubtedly doing less work, but across the average of test runs on WinXP I'm not seeing much change.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(mnoorenberghe+bmo)
Comment 3•11 years ago
|
||
Comment on attachment 774013 [details] [diff] [review] Patch Review of attachment 774013 [details] [diff] [review]: ----------------------------------------------------------------- Would this have a bigger impact for people with more extension toolbar buttons? If so, I think it's still worthwhile. ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +1685,5 @@ > parent.setAttribute("flex", Math.max(0, parentFlex - elementFlex)); > } > }, > > + isWidgetRemovable: function(aWidgetId, aOptWidgetNode) { I would prefer a single argument that can be a Node or a string containing the id. Add a comment explaining the args.
Attachment #774013 -
Flags: review?(mnoorenberghe+bmo) → review-
Updated•11 years ago
|
Flags: needinfo?(mnoorenberghe+bmo)
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #3) > Comment on attachment 774013 [details] [diff] [review] > Patch > > Review of attachment 774013 [details] [diff] [review]: > ----------------------------------------------------------------- > > Would this have a bigger impact for people with more extension toolbar > buttons? > > If so, I think it's still worthwhile. Yes, when removing widgets in buildArea we are currently always re-requesting the node even though we have a reference to it. Any add-ons that are PROVIDER_XUL hit this slow path each time isWidgetRemovable gets called. I'm going to attach two patches here that I'd like to land. One patch is to add the removable attribute to the default items that don't have the attribute. This is where we can make our defaults fast. The second patch is to make isWidgetRemovable faster by taking either an id or a node. This will make our API faster-by-default.
Comment 5•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #4) > One patch is to add the removable attribute to the default items that don't > have the attribute. This is where we can make our defaults fast. Please include a try push of just that patch to see if it's worthwhile. I would actually prefer it in a separate bug since it's independent.
Assignee | ||
Comment 6•11 years ago
|
||
Updated patch to use only one argument and check the typeof the argument.
Attachment #774013 -
Attachment is obsolete: true
Attachment #774385 -
Flags: review?(mnoorenberghe+bmo)
Assignee | ||
Updated•11 years ago
|
Attachment #774385 -
Attachment is obsolete: true
Attachment #774385 -
Flags: review?(mnoorenberghe+bmo)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #774389 -
Flags: review?(mnoorenberghe+bmo)
Comment 8•11 years ago
|
||
Comment on attachment 774389 [details] [diff] [review] Patch v2.1 Review of attachment 774389 [details] [diff] [review]: ----------------------------------------------------------------- r+ with a shorter doc. comment that focuses mostly on the argument types. ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +1688,5 @@ > > + /* > + * isWidgetRemovable takes either a widget ID or a widget node. This > + * is a performance optimization since some callers already have a > + * reference to the widget node and retrieving it again isn't necessary. A JavaDoc-style comment is more what I had in mind as it's easier to see the allowed argument types at quick glance. /** * @param {String|Node} aWidget - widget ID or a widget node (preferred for performance). * @return {Boolean} whether the widget is removable */ @@ +1692,5 @@ > + * reference to the widget node and retrieving it again isn't necessary. > + */ > + isWidgetRemovable: function(aWidget) { > + let widgetId; > + let widgetNode; Nit: You could put these on the same line: let widgetId, widgetNode;
Attachment #774389 -
Flags: review?(mnoorenberghe+bmo) → review+
Comment 9•11 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #8) > > + isWidgetRemovable: function(aWidget) { > > + let widgetId; > > + let widgetNode; > > Nit: You could put these on the same line: let widgetId, widgetNode; We don't usually do that, it arguably makes code less readable and I'm not sure why you'd do it anyway; it's not going to improve performance.
Assignee | ||
Comment 10•11 years ago
|
||
I didn't put the declaration of widgetId and widgetNode on the same line because it follows the convention that is used elsewhere. Landed with the javadoc change, https://hg.mozilla.org/projects/ux/rev/e1fd258c689b
Whiteboard: [Australis:M8][fixed-in-ux]
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e1fd258c689b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8]
Target Milestone: --- → Firefox 28
You need to log in
before you can comment on or make changes to this bug.
Description
•