Closed Bug 982322 Opened 9 years ago Closed 9 years ago

Improve template.js performance

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set
normal

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.
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)
Switching back to a "regular" for loop takes prefs load time from 639ms to 505ms (38% improvement overall).
Attachment #8392353 - Flags: review?(poirot.alex)
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)
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)
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)
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 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 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 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+
Attachment #8392366 - Flags: review?(poirot.alex) → review+
(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.
ups was to fast with closing as fixed since this landed only on m-i so far, sorry
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> This takes removing all prefs from 2963ms to 20ms (99.3% improvement / 150x faster).

Well, fuck me.
QA Whiteboard: [qa-]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.