Closed Bug 1224411 Opened 4 years ago Closed 4 years ago

Speed up FileRegistry._partial_paths

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

Details

Attachments

(1 file)

This showed up as warm in a profile for something I'm working on, and it's ~8% of install-tests.
Bug 1224411 - Speed up FileRegistry._partial_paths by memoizing on the basis of directory. r=nalexander
Attachment #8686905 - Flags: review?(nalexander)
Comment on attachment 8686905 [details]
MozReview Request: Bug 1224411 - Speed up FileRegistry._partial_paths by memoizing on the basis of directory. r=nalexander

https://reviewboard.mozilla.org/r/25101/#review22599

A few tests would be nice.

Consider making this recursive, so that the exit condition is simpler, and you get to use the cache more.
Attachment #8686905 - Flags: review?(nalexander) → review+
Attachment #8686905 - Flags: review+
Comment on attachment 8686905 [details]
MozReview Request: Bug 1224411 - Speed up FileRegistry._partial_paths by memoizing on the basis of directory. r=nalexander

https://reviewboard.mozilla.org/r/25101/#review22619

Also, while recursion sounds like a nice win, function calls in Python are relatively expensive. It's often cheaper to avoid them.

Also, aliasing objects in local values matters in tight loops. e.g.

   for x in foo:
       pkg.y.z.func(x)
       
can be written as:
   func = pkg.y.z.func
   for x in foo:
       func(x)
       
To avoid the 3 extra attribute lookups on each loop iteration.

But in this patch, it probably doesn't matter since the overhead of calling the function dwarfs the time to look up self.X foo.

::: python/mozbuild/mozpack/copier.py:45
(Diff revision 1)
> +        if dir_name in self._partial_paths_cache:
> +            return self._partial_paths_cache[dir_name]

If you are talking about micro optimizations, this performs 2 dict lookups.

The slightly more performant way to write this is:

cached = self._partial_paths_cache.get(dir_name)
if cached is not None:
    return cached
https://reviewboard.mozilla.org/r/25101/#review22599

test_copier.py has a number of checks that fail if I introduce problems in this function, but I can add some unit tests.

I'll measure the recursive version and see how we do! Thanks for the suggestion.
(In reply to Chris Manchester [:chmanchester] from comment #0)
> This showed up as warm in a profile for something I'm working on, and it's
> ~8% of install-tests.

What I mean here is that the patch for this bug saves ~8% of install-tests.
The recursive version wins by a bit (we can save ~10% of install-tests), I expect because the slow part of this function is in the repeated calls to mozpath.dirname, which using the cache more reduces.
Comment on attachment 8686905 [details]
MozReview Request: Bug 1224411 - Speed up FileRegistry._partial_paths by memoizing on the basis of directory. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25101/diff/1-2/
https://reviewboard.mozilla.org/r/25101/#review22673

Nice.  I find the recursive exit condition much easier to understand.
gps pointed out on irc that these paths are already normalized, so we can just find the last '/' instead of using dirname at all. This results in a small additional speedup.
Comment on attachment 8686905 [details]
MozReview Request: Bug 1224411 - Speed up FileRegistry._partial_paths by memoizing on the basis of directory. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25101/diff/2-3/
https://hg.mozilla.org/mozilla-central/rev/67191b9199fd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.