1224411 - Speed up FileRegistry._partial_paths

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

This showed up as warm in a profile for something I'm working on, and it's ~8% of install-tests.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1224411 - Speed up FileRegistry._partial_paths by memoizing on the basis of directory. r=nalexander

Bug 1224411 - Speed up FileRegistry._partial_paths by memoizing on the basis of directory. r=nalexander

Comment 2

[Nick Alexander :nalexander [he/him]]

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.

Comment 3

[Gregory Szorc [:gps]]

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

Comment 4

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

(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.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

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 7

[Chris Manchester (limited bugmail, email directly)]

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/

Comment 8

[Nick Alexander :nalexander [he/him]]

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

Nice.  I find the recursive exit condition much easier to understand.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

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 10

[Chris Manchester (limited bugmail, email directly)]

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/

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67191b9199fd

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/67191b9199fd