Closed Bug 1160691 Opened 5 years ago Closed 5 years ago

Optimize FrameUtils.isContent and FrameUtils.parseLocation

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

defect
Not set

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: shu, Assigned: shu)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

These helpers are currently slow.
Blocks: 1154115
Duplicate of this bug: 1160581
Attachment #8600525 - Flags: review?(jsantell)
Attached file bench.js
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
No longer blocks: 1154115
Depends on: 1154115
Blocks: perf-perf
No longer depends on: 1154115
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
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 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+
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?
https://hg.mozilla.org/mozilla-central/rev/0049538641a2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Flags: qe-verify-
Assignee: nobody → shu
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?
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 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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.