Closed
Bug 1312520
Opened 8 years ago
Closed 8 years ago
Optimize the format exposed by the manifestparser for consumption by the build system
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•