Closed
Bug 982322
Opened 10 years ago
Closed 10 years ago
Improve template.js performance
Categories
(DevTools Graveyard :: WebIDE, defect)
DevTools Graveyard
WebIDE
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: jryans, Assigned: jryans)
References
Details
Attachments
(5 files, 6 obsolete files)
23.01 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
948 bytes,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
1.43 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
1.97 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
4.82 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
template.js currently slows down quite a lot when faced with many (>1000) nodes to deal with. While Paul may indeed kill it off in the UI rewrite, I've found quite a few easy performance wins we can use right now. This is more important in support of bug 943251, as listing out all the prefs adds many more nodes than template.js dealt with before.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8391408 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8391409 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8391410 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8391411 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8391413 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8391414 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
This creates a Resolver object that navigates the object store once for movement down the tree, instead of navigating the store from root for every lookup. The test change only removes the trailing "." from the stored "rootPath" attribute, which is unnecessary. For the case of loading prefs from a device, this change takes us from 825ms to 639ms (23% improvement).
Attachment #8392347 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 8•10 years ago
|
||
Switching back to a "regular" for loop takes prefs load time from 639ms to 505ms (38% improvement overall).
Attachment #8392353 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 9•10 years ago
|
||
Similar to the last patch, using regular loops over the tree improves loading prefs from 505ms to 450ms (45% improvement overall).
Attachment #8392356 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 10•10 years ago
|
||
This is the largest improvement here. It changes an O(N^2) algorithm to O(N), by storing a node's paths on the element, instead of just looping over all possible paths for all changed nodes. This takes removing all prefs from 2963ms to 20ms (99.3% improvement / 150x faster).
Attachment #8392362 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 11•10 years ago
|
||
This delay tracking nodes until the last step during changes, which avoid double work (as I commented in the patch). For loading prefs, this takes us from 440ms to 200ms (76% improvement overall / 4x faster).
Attachment #8392366 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 12•10 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=ca34af7a645f
Comment 13•10 years ago
|
||
Comment on attachment 8392347 [details] [diff] [review] Part 1: Use a resolver object to only descend once Review of attachment 8392347 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the review delay, I got some review overflow sickness! Nice abstraction! ::: browser/devtools/app-manager/content/template.js @@ +362,5 @@ > + return obj; > + } > + let chunks = path.toString().split("."); > + for (let word of chunks) { > + if ((typeof obj) == "object" && nit: Not your code, but may be `typof(obj) == "object"` would be just easier to read.
Attachment #8392347 -
Flags: review?(poirot.alex) → review+
Comment 14•10 years ago
|
||
Comment on attachment 8392353 [details] [diff] [review] Part 2: Resolver iterates faster without for ... of Review of attachment 8392353 [details] [diff] [review]: ----------------------------------------------------------------- What?! really... that's scary!
Attachment #8392353 -
Flags: review?(poirot.alex) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8392356 [details] [diff] [review] Part 3: Process tree without for ... of Review of attachment 8392356 [details] [diff] [review]: ----------------------------------------------------------------- Ok.. ok... Then I'm wondering... don't you save some cycles by also caching the array length?
Attachment #8392356 -
Flags: review?(poirot.alex) → review+
Comment 16•10 years ago
|
||
Comment on attachment 8392362 [details] [diff] [review] Part 4: Save paths on each node Review of attachment 8392362 [details] [diff] [review]: ----------------------------------------------------------------- I don't really get these two patches, nor do I get template, but that looks good and I gave a try to these patches and that's fast!
Attachment #8392362 -
Flags: review?(poirot.alex) → review+
Updated•10 years ago
|
Attachment #8392366 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Alexandre Poirot (:ochameau) from comment #15) > Comment on attachment 8392356 [details] [diff] [review] > Part 3: Process tree without for ... of > > Review of attachment 8392356 [details] [diff] [review]: > ----------------------------------------------------------------- > > Ok.. ok... > Then I'm wondering... don't you save some cycles by also caching the array > length? This (thankfully...) makes no perceivable difference. But it's a good thought, it occurred to me as well. :) We must already optimize for this, or the length is not long enough for it to matter here.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 18•10 years ago
|
||
pushed to m-i as https://hg.mozilla.org/integration/mozilla-inbound/rev/9d661be98d68 https://hg.mozilla.org/integration/mozilla-inbound/rev/67c3aebfa9ed https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6267048082 https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b0efd466d4 https://hg.mozilla.org/integration/mozilla-inbound/rev/b45284fa40fe
Comment 19•10 years ago
|
||
ups was to fast with closing as fixed since this landed only on m-i so far, sorry
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/b45284fa40fe https://hg.mozilla.org/mozilla-central/rev/c2b0efd466d4 https://hg.mozilla.org/mozilla-central/rev/8e6267048082 https://hg.mozilla.org/mozilla-central/rev/67c3aebfa9ed https://hg.mozilla.org/mozilla-central/rev/9d661be98d68
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Comment 21•10 years ago
|
||
> This takes removing all prefs from 2963ms to 20ms (99.3% improvement / 150x faster).
Well, fuck me.
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•