Closed Bug 1203266 Opened 5 years ago Closed 1 year ago

Extract the build system's view of test metadata from the emitter and data modules to make more of it available earlier in the build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Depends on 2 open bugs)

Details

Attachments

(6 files, 5 obsolete files)

40 bytes, text/x-review-board-request
ahal
: review+
Details
40 bytes, text/x-review-board-request
ahal
: review+
Details
40 bytes, text/x-review-board-request
gps
: review+
Details
40 bytes, text/x-review-board-request
gps
: review+
Details
40 bytes, text/x-review-board-request
gps
: review+
Details
40 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1184405 suggests that some view of the build system's data on tests should be readily available without a build (and without hacks to run part of the emitter outside of a build). This could mean reading test manifests as a part of the build reader, extracting more parts of the emitter concerned with reading test manifests, or even incorporating the manifestparser into the build system in some way.
Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps
Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

f? only for now because I'm a bit unsure of the direction here (although it is effective).

Running mach build-backend a few times, this consistently moves 200ms of reported time from the "Processed into <count> build config descriptors in" section to the "Finished reading <count> moz.build files in" section, with the overall times remaining comparable.

If this is generally on the right track I'll extend it to the other manifest types and thread it through the files_info code.
Attachment #8669165 - Flags: feedback?(gps)
https://reviewboard.mozilla.org/r/21135/#review19029

Yes, moving things from emitter to reader is something we're generally interested in. And this patch does it well.

Please note the special Mercurial-based reading mode for moz.build files: we support reading and evaluating moz.build files directly from a Mercurial repository, without any working copy / checkout. We had to change a number of places that assumed we were dealing with files, not file handles. There is an abstracted I/O layer that you should go through to ensure the test manifest parser can read content from Mercurial.

On that front, reading test manifests at moz.build time does add some overhead. This is one place where the separation between reader and emitter is beneficial. I'd be much happier if test manifests didn't take as long to read. Perhaps you could profile that while you are mucking about with test manifest handling :)

::: python/mozbuild/mozbuild/frontend/context.py:539
(Diff revision 1)
> +    class ListWithAction(ContextDerivedValue, List):

This feels like something that should be inthe base class. Perhaps you can pass a callable to its __init__ that gets called when defined and gives the opportunity to mutate the value?

::: python/mozbuild/mozbuild/frontend/context.py:569
(Diff revision 1)
> +ManifestparserTestManifestList = OrderedListWithActionFactory(read_single_manifest)

This is elegant and I like it!
(In reply to Gregory Szorc [:gps] from comment #3)
> 
> On that front, reading test manifests at moz.build time does add some
> overhead. This is one place where the separation between reader and emitter
> is beneficial. I'd be much happier if test manifests didn't take as long to
> read. Perhaps you could profile that while you are mucking about with test
> manifest handling :)

I checked into this, only prominent thing in the profile was a call to normpath taking about 25% of the time spent reading manifests. Unfortunately, this is called for every test path in every manifest, but removing it would certainly change output.

On my laptop reading all manifestparser manifests takes around 190ms (~3% of |./mach build-backend|).
See bug 1191575 for some analysis about the test manifest parsing thing
(In reply to Mike Hommey [:glandium] from comment #5)
> See bug 1191575 for some analysis about the test manifest parsing thing

comments 5 to 8.
Attachment #8669165 - Flags: feedback?(gps)
This is comment 3 for wpt manifests, the commit message has a summary.
Attachment #8672054 - Flags: review?(james)
(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #5)
> > See bug 1191575 for some analysis about the test manifest parsing thing
> 
> comments 5 to 8.

I don't see a reason not to use the "if '..' in path" optimization mentioned there (it will change the result if test paths have "//" or "/./" in them, but I don't think that matters).
Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

Bug 1203266 - Don't call normpath in the manifestparser on paths that don't contain '..'. r=ahal

Profiling the manifestparser revealed that 25% of its total time is spent in a single
call to os.path.normpath, called on each test path in each manifest. For the manifests
in mozilla-central, these calls almost always return their input. To save time, this
patch modifies the manifestparser to not call normpath on paths that don't contain
'..'. This will change the parsed result of manifests containing no-op path
components ('//' or '/./') to include those components in their output.
Attachment #8669165 - Attachment description: MozReview Request: Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps → MozReview Request: Bug 1203266 - Don't call normpath in the manifestparser on paths that don't contain '..'. r=ahal
Attachment #8669165 - Flags: review?(ahalberstadt)
Bug 1203266 - Optionally read manifestparser manifests with a Finder class. r=ahal

We need to go through an abstraction layer when manifestparser manifests
are read as a part of reading moz.build files in case moz.build reading
is using mercurial as its filesystem. This adds an optional finder member,
which will be used if present for IO that happens when reading manifests,
This needs to be optional, because the manifestparser can be used as a
standalone package that isn't distributed with mozpack.
Attachment #8672059 - Flags: review?(ahalberstadt)
Bug 1203266 - Allow reading reftest manifests from a provided finder. r=gps
Attachment #8672060 - Flags: review?(gps)
Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps
Attachment #8672072 - Flags: review?(gps)
Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps
Attachment #8672073 - Flags: review?(gps)
Bug 1203266 - Remove uses of the emitter in files_info now that they're no longer needed. r=gps
Attachment #8672074 - Flags: review?(gps)
Comment on attachment 8672054 [details] [review]
PR to allow passing a file-like object to the wpt manifest reader

This has been merged, thank you.
Attachment #8672054 - Flags: review?(james)
Attachment #8672054 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/21135/#review19537

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:157
(Diff revision 2)
>                  path = normalize_path(path)

It appears we're doing path normalization up here. Could we not perform the os.path.join() above and always do the normalization below? That should make this extra normpath() unnecessary and have the same effect as this patch.
Comment on attachment 8672060 [details]
MozReview Request: Bug 1203266 - Allow reading reftest manifests from a provided finder. r=gps

https://reviewboard.mozilla.org/r/21509/#review19539

::: layout/tools/reftest/reftest/__init__.py:61
(Diff revision 1)
> +        if self.finder:
> +            lines = self.finder.get(path).read().splitlines()
> +        else:
> -        with open(path, 'r') as fh:
> +            with open(path, 'r') as fh:
> +                lines = fh.read().splitlines()

A subtle, possibly-not-relevant difference in this code is that `for line in fh` returns the line with the newline char(s) and `splitline()` strips the newline char(s).

Since we're doing a line.strip() below, it almost certainly doesn't matter.

::: layout/tools/reftest/reftest/__init__.py:62
(Diff revision 1)
> +            lines = self.finder.get(path).read().splitlines()

I don't like scope bloating, but given this and the previous patch, we may want to implement readline() on Finder instances.
Attachment #8672060 - Flags: review?(gps) → review+
Comment on attachment 8672072 [details]
MozReview Request: Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps

https://reviewboard.mozilla.org/r/21667/#review19541

::: python/mozbuild/mozbuild/util.py:266
(Diff revision 1)
> -    def __init__(self, iterable=[]):
> +    def __init__(self, iterable=[], **kwargs):

While you are here, using [] as default arguments is bad. See http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments.

We're likely not running into any problems because we're not relying on the default value in any callers. Still, this is a giant land mine waiting to strike.

::: python/mozbuild/mozbuild/util.py:385
(Diff revision 1)
> +class ListWithActionMixin(object):

Please add a docstring.

::: python/mozbuild/mozbuild/util.py:386
(Diff revision 1)
> +    def __init__(self, iterable=[], action=None):

action appears to be required. So no need to use a default value.

::: python/mozbuild/mozbuild/util.py:389
(Diff revision 1)
> +        return super(ListWithActionMixin, self).__init__(iterable)

You don't need to return anything from __init__ functions. Not sure why this is done in other classes. Please don't cargo cult it.

(__new__ is the function that needs to return something. In fact, when you use __new__, a non-None return by __init__ will raise TypeError!)

::: python/mozbuild/mozbuild/util.py:401
(Diff revision 1)
> +        import pdb; pdb.set_trace()

pdb?!
Attachment #8672072 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/21135/#review19537

> It appears we're doing path normalization up here. Could we not perform the os.path.join() above and always do the normalization below? That should make this extra normpath() unnecessary and have the same effect as this patch.

The "normalize_path" function in the manifestparser only replaces separators, it doesn't collapse ..'s (why that is, I don't know).
https://reviewboard.mozilla.org/r/21667/#review19541

> action appears to be required. So no need to use a default value.

But changing the order of parameters would do weirdness. I'll raise if it isn't provided.

> pdb?!

Oh, right. This method (and StrictOrderingOnAppendListMixin's __add__) are dead code, because ListMixin's __add__ never calls super.
Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

https://reviewboard.mozilla.org/r/21135/#review19605
Attachment #8669165 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8672059 [details]
MozReview Request: [mq]: dummy2

https://reviewboard.mozilla.org/r/21507/#review19607

Looks good to me, but have a few questions and doc cleanup before giving an r+

::: testing/mozbase/manifestparser/manifestparser/ini.py:32
(Diff revision 1)
> -    for (linenum, line) in enumerate(fp.readlines(), start=1):
> +    for (linenum, line) in enumerate(fp.read().splitlines(), start=1):

Why is read() + splitlines() better than readlines() ?

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:47
(Diff revision 1)
> -    def __init__(self, manifests=(), defaults=None, strict=True, rootdir=None):
> +    def __init__(self, manifests=(), defaults=None, strict=True, rootdir=None,
> +                 finder=None):

Would you mind adding a docstring here while you're changing the signature anyway?

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:100
(Diff revision 1)
> +            # If we're using mercurial as our filesystem via a finder
> +            # during manifest reading, the getcwd() calls that happen
> +            # with abspath calls will not be meaningful, so absolute
> +            # paths are required.

This comment doesn't make sense to me. Also what does mercurial have to do with anything?

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py:107
(Diff revision 1)
> -            fp = open(filename)
> +            fp = self.get_fp(filename)

nit: if this is the only place it's called, might as well inline 'get_fp'.
Attachment #8672059 - Flags: review?(ahalberstadt)
https://reviewboard.mozilla.org/r/21507/#review19607

> Why is read() + splitlines() better than readlines() ?

"fp" is going to be a File object from python/mozbuild/mozpack/files.py, and they don't implement readlines() (although I may just fix that).

> Would you mind adding a docstring here while you're changing the signature anyway?

I'll try!

> This comment doesn't make sense to me. Also what does mercurial have to do with anything?

One of the variants of FileFinder is MercurialFinder, which backs reads with hg, so I don't think getcwd is going to be what we want.
Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

Bug 1203266 - Don't call normpath in the manifestparser on paths that don't contain '..'. r=ahal

Profiling the manifestparser revealed that 25% of its total time is spent in a single
call to os.path.normpath, called on each test path in each manifest. For the manifests
in mozilla-central, these calls almost always return their input. To save time, this
patch modifies the manifestparser to not call normpath on paths that don't contain
'..'. This will change the parsed result of manifests containing no-op path
components ('//' or '/./') to include those components in their output.
Comment on attachment 8672059 [details]
MozReview Request: [mq]: dummy2

Bug 1203266 - Optionally read manifestparser manifests with a Finder class. r=ahal

We need to go through an abstraction layer when manifestparser manifests
are read as a part of reading moz.build files in case moz.build reading
is using mercurial as its filesystem. This adds an optional finder member,
which will be used if present for IO that happens when reading manifests,
This needs to be optional, because the manifestparser can be used as a
standalone package that isn't distributed with mozpack.
Attachment #8672059 - Flags: review?(ahalberstadt)
Attachment #8672060 - Attachment is obsolete: true
Attachment #8672072 - Attachment is obsolete: true
Attachment #8672073 - Attachment is obsolete: true
Attachment #8672073 - Flags: review?(gps)
Attachment #8672074 - Attachment is obsolete: true
Attachment #8672074 - Flags: review?(gps)
Attachment #8672059 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

Bug 1203266 - Don't call normpath in the manifestparser on paths that don't contain '..'. r=ahal

Profiling the manifestparser revealed that 25% of its total time is spent in a single
call to os.path.normpath, called on each test path in each manifest. For the manifests
in mozilla-central, these calls almost always return their input. To save time, this
patch modifies the manifestparser to not call normpath on paths that don't contain
'..'. This will change the parsed result of manifests containing no-op path
components ('//' or '/./') to include those components in their output.
Comment on attachment 8672059 [details]
MozReview Request: [mq]: dummy2

Bug 1203266 - Optionally read manifestparser manifests with a Finder class. r=ahal

We need to go through an abstraction layer when manifestparser manifests
are read as a part of reading moz.build files in case moz.build reading
is using mercurial as its filesystem. This adds an optional finder member,
which will be used if present for IO that happens when reading manifests,
This needs to be optional, because the manifestparser can be used as a
standalone package that isn't distributed with mozpack.
Bug 1203266 - Allow reading reftest manifests from a provided finder. r=gps
Attachment #8674585 - Flags: review?(gps)
Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps
Attachment #8674586 - Flags: review?(gps)
Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps
Attachment #8674587 - Flags: review?(gps)
Bug 1203266 - Remove uses of the emitter in files_info now that they're no longer needed. r=gps
Attachment #8674588 - Flags: review?(gps)
Comment on attachment 8674585 [details]
MozReview Request: [mq]: dummy3

This was already reviewed. Pushing part of the series earlier skewed things.
Attachment #8674585 - Flags: review?(gps) → review+
Comment on attachment 8674586 [details]
MozReview Request: Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps

https://reviewboard.mozilla.org/r/22261/#review19923

::: python/mozbuild/mozbuild/util.py:267
(Diff revision 1)
> +        if iterable is None:
> +            iterable = []

This /could/ be written as `iterable = iterable or []`. But checking against the explicit default argument value is OK (and arguably more correct).

::: python/mozbuild/mozbuild/util.py:395
(Diff revision 1)
> +        if action is None:
> +            raise ValueError('A callabe action is required to construct '
> +                             'a ListWithAction')

if not callable(action):

::: python/mozbuild/mozbuild/util.py:429
(Diff revision 1)
> +    A callable (action) may optionally be passed to the constructor to run on

You say this is optional but LisWithActionMixin() requires action isn't None.
Attachment #8674586 - Flags: review?(gps) → review+
Attachment #8674587 - Flags: review?(gps)
Comment on attachment 8674587 [details]
MozReview Request: Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

https://reviewboard.mozilla.org/r/22263/#review19941

This is good except for the injection of new dependencies into the reader. You sadly need to bloat scope to fix the packaging situation for these manifest parsers :/

::: python/mozbuild/mozbuild/frontend/context.py:558
(Diff revision 1)
> +    class _TypedListWithAction(ContextDerivedValue, TypedList(typ), ListWithAction):
> +        def __init__(self, context, *args):
> +            def _action(item):
> +                return item, action(context, item)
> +            super(_TypedListWithAction, self).__init__(action=_action, *args)

Your Python skills are strong.

::: python/mozbuild/mozbuild/testing.py:18
(Diff revision 1)
> +import reftest

This is going to introduce a new dependency that AFAIK isn't officially packaged. This will require a little extra work for hg.mozilla.org.

We should have a followup to get the reftest manifest parser integrated into the more formal Python packages.

::: python/mozbuild/mozbuild/testing.py:312
(Diff revision 1)
> +        execfile(paths_file, _globals)

This will break in Mercurial reading mode because it is relying on the filesystem to read content.

::: python/mozbuild/mozbuild/testing.py:313
(Diff revision 1)
> +        import manifest as wptmanifest

Same issue as reftest Python modules.
Comment on attachment 8674588 [details]
MozReview Request: Bug 1203266 - Remove uses of the emitter in files_info now that they're no longer needed. r=gps

https://reviewboard.mozilla.org/r/22265/#review19943
Attachment #8674588 - Flags: review?(gps) → review+
Depends on: 1215709
I'll land the first parts now until we figure out packaging.
Keywords: leave-open
https://reviewboard.mozilla.org/r/22263/#review19941

> This will break in Mercurial reading mode because it is relying on the filesystem to read content.

As discussed on irc and in bug 1252798, I think we already expose this hazard (because calling files_info will run the emitter in order to read manifests).
Comment on attachment 8669165 [details]
MozReview Request: [mq]: dummy1

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21135/diff/4-5/
Attachment #8669165 - Attachment description: MozReview Request: Bug 1203266 - Don't call normpath in the manifestparser on paths that don't contain '..'. r=ahal → MozReview Request: [mq]: dummy1
Comment on attachment 8672059 [details]
MozReview Request: [mq]: dummy2

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21507/diff/3-4/
Attachment #8672059 - Attachment description: MozReview Request: Bug 1203266 - Optionally read manifestparser manifests with a Finder class. r=ahal → MozReview Request: [mq]: dummy2
Comment on attachment 8674585 [details]
MozReview Request: [mq]: dummy3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22259/diff/1-2/
Attachment #8674585 - Attachment description: MozReview Request: Bug 1203266 - Allow reading reftest manifests from a provided finder. r=gps → MozReview Request: [mq]: dummy3
Attachment #8674585 - Flags: review+
Comment on attachment 8674586 [details]
MozReview Request: Bug 1203266 - Add a list variant to mozbuild.util that allows pre-processing list items with a provided callable. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22261/diff/1-2/
Comment on attachment 8674587 [details]
MozReview Request: Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22263/diff/1-2/
Attachment #8674587 - Flags: review?(gps)
Comment on attachment 8674588 [details]
MozReview Request: Bug 1203266 - Remove uses of the emitter in files_info now that they're no longer needed. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22265/diff/1-2/
Comment on attachment 8674587 [details]
MozReview Request: Bug 1203266 - Read test manifests in the reader instead of the emitter to make them available earlier in the build. r=gps

https://reviewboard.mozilla.org/r/22263/#review34531

::: python/mozbuild/mozbuild/frontend/context.py:603
(Diff revision 2)
> +    invokes the given given callable with each input and a context as it is

Nit: "given given"

::: python/mozbuild/mozbuild/frontend/context.py:619
(Diff revision 2)
> +ManifestparserManifestList = OrderedListWithAction(read_manifestparser_manifest)
> +ReftestManifestList = OrderedListWithAction(read_reftest_manifest)
> +WptManifestList = TypedListWithAction(WebPlatformTestManifest, read_wpt_manifest)

One thing that could bite us with these WithAction types is that the item assigned to the list is no longer what you think. As long as we're not hanging flags off items or searching for items from moz.build, I think we'll be fine.

::: python/mozbuild/mozbuild/testing.py:311
(Diff revision 2)
> +    old_path = sys.path[:]
> +    try:
> +        # Setup sys.path to include all the dependencies required to import
> +        # the web-platform-tests manifest parser. web-platform-tests provides
> +        # a the localpaths.py to do the path manipulation, which we load,
> +        # providing the __file__ variable so it can resolve the relative
> +        # paths correctly.
> +        paths_file = os.path.join(context.config.topsrcdir, "testing",
> +                                  "web-platform", "tests", "tools", "localpaths.py")
> +        _globals = {"__file__": paths_file}
> +        execfile(paths_file, _globals)
> +        import manifest as wptmanifest
> +    finally:
> +        sys.path = old_path

Congratulations on becoming the blame owner for this code!
Attachment #8674587 - Flags: review?(gps) → review+
Duplicate of this bug: 1252798
\o/

Does this make bugs like bug 1209248 obsolete? Or will there be some follow up work for things like |mach try| to reap the benefits?
Sorry that's not the bug I was thinking of. I'm asking if |mach try| will work without a build now when you specify test paths.
(In reply to Andrew Halberstadt [:ahal] from comment #52)
> Sorry that's not the bug I was thinking of. I'm asking if |mach try| will
> work without a build now when you specify test paths.

Not quite, because the test resolver uses all-tests.json, which is written by the build backend, but having a resolver that works from file system reading mode is much easier to imagine now.
Product: Core → Firefox Build System
The leave-open keyword is there and there is no activity for 6 months.
:kmoir, maybe it's time to close this bug?
Flags: needinfo?(kmoir)
Chris, it seems like you worked extensively on this bug.  Can it be closed or should it be left open as per the leave-open keyword?  Is there still work left to do or is this an inactive bug?
Flags: needinfo?(kmoir) → needinfo?(cmanchester)
I'm pretty sure everything relevant here landed.
Assignee: nobody → cmanchester
Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(cmanchester)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.