Closed
Bug 1160691
Opened 9 years ago
Closed 9 years ago
Optimize FrameUtils.isContent and FrameUtils.parseLocation
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(firefox40 fixed, firefox41 fixed)
RESOLVED
FIXED
Firefox 41
People
(Reporter: shu, Assigned: shu)
References
Details
Attachments
(3 files)
9.48 KB,
patch
|
jsantell
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
6.69 KB,
application/javascript
|
Details | |
4.68 KB,
patch
|
Details | Diff | Splinter Review |
These helpers are currently slow.
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8600525 -
Flags: review?(jsantell)
Assignee | ||
Comment 3•9 years ago
|
||
Here's the benchmark program I wrote. The numbers: parseLocation took 497 ms parseLocationOld took 2343 ms isContent took 367 ms isContentOld took 1942 ms
Assignee | ||
Updated•9 years ago
|
Updated•9 years ago
|
Comment 4•9 years ago
|
||
Comment on attachment 8600525 [details] [diff] [review] Optimize FrameUtils.isContent and FrameUtils.parseLocation. Review of attachment 8600525 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/profiler/frame-utils.js @@ +8,5 @@ > loader.lazyRequireGetter(this, "Services"); > loader.lazyRequireGetter(this, "CATEGORY_OTHER", > "devtools/shared/profiler/global", true); > > +// Character codes used in various parsing helper functions. Should add a header comment up here for the file saying why this is written the way it is (for performance reasons) and because it's very hot code, and list some of the speed up improvements with profiler data size
Comment 5•9 years ago
|
||
Tests for this patch, works on current frame stuff, so it should work once everything else is landed (may need to modify the argify function)
Comment 6•9 years ago
|
||
Comment on attachment 8600525 [details] [diff] [review] Optimize FrameUtils.isContent and FrameUtils.parseLocation. Review of attachment 8600525 [details] [diff] [review]: ----------------------------------------------------------------- Good once everything else lands in bug 1154115 and give it a go on try with this test
Attachment #8600525 -
Flags: review?(jsantell) → review+
Comment 7•9 years ago
|
||
wait this bench mark was for 1 000 000 and 5 000 000 "fn (http://...)" strings -- is that an accurate number of /unique/ frames, especially after deduping?
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0049538641a2
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•9 years ago
|
Blocks: perf-40-uplifts
Updated•9 years ago
|
Flags: qe-verify-
Updated•9 years ago
|
Assignee: nobody → shu
Comment 10•9 years ago
|
||
Comment on attachment 8600525 [details] [diff] [review] Optimize FrameUtils.isContent and FrameUtils.parseLocation. Approval Request Comment [Feature/regressing bug #]: 1167252, the new performance tool [User impact if declined]: Won't ship the performance tool [Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift [Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake. Risks are generally contained within devtools, specifically within the performance panel. [String/UUID change made/needed]: None
Attachment #8600525 -
Flags: approval-mozilla-aurora?
Comment 11•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ea3d70cbe275
status-firefox40:
--- → fixed
Comment 12•9 years ago
|
||
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+. See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment 13•9 years ago
|
||
Comment on attachment 8600525 [details] [diff] [review] Optimize FrameUtils.isContent and FrameUtils.parseLocation. Change approved to skip one train as part of the spring campaign.
Attachment #8600525 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•