Closed Bug 1312520 Opened 3 years ago Closed 3 years ago

Optimize the format exposed by the manifestparser for consumption by the build system

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

Attachments

(3 files)

Reading manifestparser manifests results in propagating definitions from manifest defaults to individual test objects, resulting in individual test objects that each provide a complete account of when and how to run that test. Unfortunately, the way the build system used this format we end up doing some somewhat expensive and redundant operations for every item for every test. This accounts for most of the time spent in the emitter.

By adding an option to the manifestparser to expose manifest defaults separately, we can get rid of most of these redundant operations.
Blocks: 1312574
Comment on attachment 8804028 [details]
Bug 1312520 - Add an option to the manifestparser to prevent defaults from propagating to individual section definitions.

https://reviewboard.mozilla.org/r/88176/#review87172

LGTM. But ahal knows this code better than me and you should wait for his review.
Attachment #8804028 - Flags: review+
Comment on attachment 8804029 [details]
Bug 1312520 - Extract the logic for combining defaults and individual section defnitions in the manifestparser to a standalone function.

https://reviewboard.mozilla.org/r/88178/#review87174
Attachment #8804029 - Flags: review+
Comment on attachment 8804030 [details]
Bug 1312520 - Store and process manifest-level defaults in the build system separately from individual tests.

https://reviewboard.mozilla.org/r/88180/#review87178
Attachment #8804030 - Flags: review?(gps) → review+
Comment on attachment 8804028 [details]
Bug 1312520 - Add an option to the manifestparser to prevent defaults from propagating to individual section definitions.

https://reviewboard.mozilla.org/r/88176/#review87354

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:69
(Diff revision 1)
> +        :param omit_defaults: If set, do not propagate manifest defaults to individual
> +                              test objects. Callers are expected to manage per-manifest
> +                              defaults themselves via the manifest_defaults member
> +                              variable in this case.

Just also call this "handle_defaults", it's mildly less confusing.
Attachment #8804028 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8804029 [details]
Bug 1312520 - Extract the logic for combining defaults and individual section defnitions in the manifestparser to a standalone function.

https://reviewboard.mozilla.org/r/88178/#review87356

::: testing/mozbase/manifestparser/manifestparser/expression.py:327
(Diff revision 1)
> +def combine_fields(global_vars, local_vars):
> +    """
> +    Combine the given manifest entries according to the semantics of specific fields.
> +    This is used to combine manifest level defaults with a per-test definition.
> +    """

I think this still belongs in ini.py.

::: testing/mozbase/manifestparser/manifestparser/expression.py:340
(Diff revision 1)
> +    final_mapping = global_vars.copy()
> +    for field_name, value in local_vars.items():
> +        if field_name not in field_patterns or field_name not in global_vars:
> +            final_mapping[field_name] = value
> +            continue
> +        global_value = global_vars[field_name]
> +        pattern = field_patterns[field_name]
> +        final_mapping[field_name] = pattern % (
> +            global_value.split('#')[0], value.split('#')[0])

Was there a particular reason to change the implementation? It looks fine, just curious if this is supposed to be faster or something.
Attachment #8804029 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8804030 [details]
Bug 1312520 - Store and process manifest-level defaults in the build system separately from individual tests.

https://reviewboard.mozilla.org/r/88180/#review87366

So I have a patch for bug 1293259 (locally) that I've been slowly chipping away at that stops using all-tests.json altogether and uses manifestparser more directly. It still uses the build system to generate the set of root manifests from moz.build though. I think there is some overlap between these two patches, but go ahead and land this, I'll rebase on top and be sure to flag you for feedback when I have something a bit more fleshed out.
Comment on attachment 8804029 [details]
Bug 1312520 - Extract the logic for combining defaults and individual section defnitions in the manifestparser to a standalone function.

https://reviewboard.mozilla.org/r/88178/#review87356

> Was there a particular reason to change the implementation? It looks fine, just curious if this is supposed to be faster or something.

I don't think it's faster, it just seemed a bit clearer.
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dda6b3c96e0
Add an option to the manifestparser to prevent defaults from propagating to individual section definitions. r=ahal
https://hg.mozilla.org/integration/mozilla-inbound/rev/11a7683f82d1
Extract the logic for combining defaults and individual section defnitions in the manifestparser to a standalone function. r=ahal
https://hg.mozilla.org/integration/mozilla-inbound/rev/182325e65926
Store and process manifest-level defaults in the build system separately from individual tests. r=gps
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.