1355630 - Performance improvements to files info resolution

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

Bug 1352495 is doing some things that is finding all of the algorithmic inefficiencies in files metadata reading code. Let's make some tweaks to help with performance.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1355630 - Use a set for membership testing;

With ~42,000 entries in relpaths, this change drops execution time
of this loop from ~23s to ~0.01s.

Review commit: https://reviewboard.mozilla.org/r/129182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129182/

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1355630 - Extract set declaration to member variable;

The variable is deterministic. We can call this function thousands of
times. There's no need to populate the set on every invocation.

Review commit: https://reviewboard.mozilla.org/r/129184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129184/

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1355630 - Memoize tests_defaults_for_path call;

Calling self.test_defaults_for_path() from files_info() with tens
of thousands of paths resulted in a CPU explosion in various path
normalization functions. I don't think it was so much the complexity
of the operations as much as the volume.

For a simple input of 100 elements, this reduces execution time of
a mach command from ~25s to ~4s. With ~42,000 inputs, execution time
drops from effectively infinite to ~90s.

Review commit: https://reviewboard.mozilla.org/r/129186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129186/

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1355630 - Inline specialized version of relpath();

Profiling revealed that mozpath.relpath() accounted for a lot of CPU
time when operating on an input of ~42,000 paths.

Due to the nature of the paths we're operating on, we don't need the
full power of mozpath.relpath() here. Instead, we can implement a
specialized version that works given already normalized paths and the
knowledge that context paths must be ancestors of the current path
being examined.

This change drops execution time of a mach command feeding ~42,000
paths to this function from ~90s to ~24s.

Review commit: https://reviewboard.mozilla.org/r/129188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129188/

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8857243 [details]
Bug 1355630 - Memoize tests_defaults_for_path call;

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

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8857244 [details]
Bug 1355630 - Inline specialized version of relpath();

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

Comment 7

[Stefan Sitter [:ssitter]]

Should this be in product Core instead of Calendar?

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8857241 [details]
Bug 1355630 - Use a set for membership testing;

https://reviewboard.mozilla.org/r/129182/#review132218

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8857242 [details]
Bug 1355630 - Extract set declaration to member variable;

https://reviewboard.mozilla.org/r/129184/#review132222

::: python/mozbuild/mozbuild/frontend/reader.py:1403
(Diff revision 1)
> -        test_manifest_contexts = set(
> +    TEST_MANIFEST_CONTEXTS = set(
> -            ['%s_MANIFESTS' % key for key in TEST_MANIFESTS] +
> +        ['%s_MANIFESTS' % key for key in TEST_MANIFESTS] +
> -            ['%s_MANIFESTS' % flavor.upper() for flavor in REFTEST_FLAVORS] +
> +        ['%s_MANIFESTS' % flavor.upper() for flavor in REFTEST_FLAVORS] +
> -            ['%s_MANIFESTS' % flavor.upper().replace('-', '_') for flavor in WEB_PLATFORM_TESTS_FLAVORS]
> +        ['%s_MANIFESTS' % flavor.upper().replace('-', '_') for flavor in
> +         WEB_PLATFORM_TESTS_FLAVORS]
> -        )
> +    )
>  
> +    def test_defaults_for_path(self, ctxs):

Adding this global is kinda ugly. This is the only patch here that doesn't have timings in the commit message... what's the actual overhead?

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8857243 [details]
Bug 1355630 - Memoize tests_defaults_for_path call;

https://reviewboard.mozilla.org/r/129186/#review132226

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8857244 [details]
Bug 1355630 - Inline specialized version of relpath();

https://reviewboard.mozilla.org/r/129188/#review132232

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8857242 [details]
Bug 1355630 - Extract set declaration to member variable;

https://reviewboard.mozilla.org/r/129184/#review132222

> Adding this global is kinda ugly. This is the only patch here that doesn't have timings in the commit message... what's the actual overhead?

I'll just drop this patch. I don't think it matters too much.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8857243 [details]
Bug 1355630 - Memoize tests_defaults_for_path call;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129186/diff/2-3/

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8857244 [details]
Bug 1355630 - Inline specialized version of relpath();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129188/diff/2-3/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3af9177e369
Use a set for membership testing; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/e9e8c13f08ad
Memoize tests_defaults_for_path call; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/d9a08a6f56c9
Inline specialized version of relpath(); r=chmanchester

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c3af9177e369
https://hg.mozilla.org/mozilla-central/rev/e9e8c13f08ad
https://hg.mozilla.org/mozilla-central/rev/d9a08a6f56c9