If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 52

Status

()

Core
Build Config
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

11 months ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Blocks: 1312574

Comment 4

11 months ago
mozreview-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/#review87172

LGTM. But ahal knows this code better than me and you should wait for his review.
Attachment #8804028 - Flags: review+

Comment 5

11 months ago
mozreview-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 6

11 months ago
mozreview-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 7

11 months ago
mozreview-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 8

11 months ago
mozreview-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 9

11 months ago
mozreview-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.
(Assignee)

Comment 10

11 months ago
mozreview-review-reply
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.

Comment 11

11 months ago
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

Comment 12

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1dda6b3c96e0
https://hg.mozilla.org/mozilla-central/rev/11a7683f82d1
https://hg.mozilla.org/mozilla-central/rev/182325e65926
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1313716
You need to log in before you can comment on or make changes to this bug.