Closed Bug 1431229 Opened 4 years ago Closed 4 years ago

Move some webidl/ipdl processing from the common backend to the emitter

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

Attachments

(3 files)

I'd like to get a full account of the object files the build is going to produce from the emitter, so that by the time a backend sees a linkable object it knows everything that goes into it. Some source file names aren't established until we do some processing of webidls and ipdls, so this processing needs to move to the emitter.
Does this mean we'll be reading IDL files during `mach build-backend`? If so, there are performance implications, since e.g. parsing WebIDL files requires several seconds of CPU in the current implementation. I'm generally in favor of doing this processing sooner, as IDL processing is a long pole in the export tier and it slows down builds on high core count machines by several seconds by delaying the transition from export to compile tiers. We just need to make sure `mach build-backend` is still fast for minor changes that don't touch IDL configuration.
Comment on attachment 8944626 [details]
Bug 1431229 - Populate WebIDLCollection from the emitter rather than the common backend.

https://reviewboard.mozilla.org/r/214768/#review220660
Attachment #8944626 - Flags: review+
Comment on attachment 8944625 [details]
Bug 1431229 - Add configure variables to specify the webidl and ipdl root directories.

https://reviewboard.mozilla.org/r/214766/#review220658

::: toolkit/moz.configure:1199
(Diff revision 2)
> +def idl_roots(build_env):
> +    return namespace(ipdl_root=os.path.join(build_env.topobjdir, 'ipc', 'ipdl'),
> +                     webidl_root=os.path.join(build_env.topobjdir,
> +                                              'dom', 'bindings'))
> +
> +set_config('WEBIDL_ROOT', idl_roots.webidl_root)

I don't think these really make sense in configure, since they're more like constants rather than variables that would be changed via a mozconfig. Is there somewhere in mozbuild we could put them? I do think having them defined in some central place is a good idea, since we currently hardcode 'dom/bindings' and such in the places where it's needed, but configure feels like the wrong location to me.
Attachment #8944625 - Flags: review?(core-build-config-reviews)
Comment on attachment 8944627 [details]
Bug 1431229 - Populate IPDLCollection from the emitter rather than the common backend.

https://reviewboard.mozilla.org/r/214770/#review220662

::: python/mozbuild/mozbuild/backend/common.py
(Diff revision 2)
> -                              '%sParent.cpp' % root])
> -            return files
> -
> -        ipdl_dir = mozpath.join(self.environment.topobjdir, 'ipc', 'ipdl')
> -
> -        ipdl_cppsrcs = list(itertools.chain(*[files_from(p) for p in sorted_ipdl_sources]))

You can remove the 'import itertools' from this file now.
Attachment #8944627 - Flags: review+
Attachment #8944627 - Flags: review?(core-build-config-reviews)
Attachment #8944626 - Flags: review?(core-build-config-reviews)
(In reply to Gregory Szorc [:gps] from comment #2)
> Does this mean we'll be reading IDL files during `mach build-backend`? If
> so, there are performance implications, since e.g. parsing WebIDL files
> requires several seconds of CPU in the current implementation.

Fortunately no - this is simplifying the mozbuild internal interface for these types of files. Instead of the emitting ~7 different WebIDL objects that the backend consumes and assembles into a complete picture of the WebIDL processing, there will be a single WebIDL object that is emitted (and similar for IPDL). Neither WebIDL/IPDL need to be parsed for this.
Comment on attachment 8944625 [details]
Bug 1431229 - Add configure variables to specify the webidl and ipdl root directories.

https://reviewboard.mozilla.org/r/214766/#review220658

> I don't think these really make sense in configure, since they're more like constants rather than variables that would be changed via a mozconfig. Is there somewhere in mozbuild we could put them? I do think having them defined in some central place is a good idea, since we currently hardcode 'dom/bindings' and such in the places where it's needed, but configure feels like the wrong location to me.

My first version of this actually set them in moz.build (dom/bindings/moz.build would set `IPDL_ROOT = True` there -- ), but the code to enforce setting these only once in the tree was sort of tedious. Doing this in configure enforces the one value per tree semantics and simplifies the implementation significantly. Note in this implementation nothing depends on a mozconfig, these are effectively constants.

Does the moz.build approach seem better?
needinfo for comment 13 so this doesn't stall out
Flags: needinfo?(mshal)
Is there any reason we can't just define things as constants in the emitter?
(In reply to Nathan Froyd [:froydnj] from comment #15)
> Is there any reason we can't just define things as constants in the emitter?

Not really, although it is convenient to be able to override the values in tests. Having the emitter verify we usually require a dom/bindings and ipc/ipdl to be in the tree becomes slightly awkward because js builds and tests don't care about these.
(In reply to Nathan Froyd [:froydnj] from comment #15)
> Is there any reason we can't just define things as constants in the emitter?

I was thinking something like this (or some common place like base.py - anything under python/mozbuild/mozbuild/) rather than an individual moz.build file. If that makes things too difficult to do the test cases properly, I'd prefer to have the tests and leave it in configure instead of moving it out and killing some test cases.
Flags: needinfo?(mshal)
(In reply to Chris Manchester (:chmanchester) from comment #16)
> (In reply to Nathan Froyd [:froydnj] from comment #15)
> > Is there any reason we can't just define things as constants in the emitter?
> 
> Not really, although it is convenient to be able to override the values in
> tests. Having the emitter verify we usually require a dom/bindings and
> ipc/ipdl to be in the tree becomes slightly awkward because js builds and
> tests don't care about these.

These seem like decent reasons to put things in configure.
Comment on attachment 8944625 [details]
Bug 1431229 - Add configure variables to specify the webidl and ipdl root directories.

https://reviewboard.mozilla.org/r/214766/#review221058
Attachment #8944625 - Flags: review+
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5df9791ef3b
Add configure variables to specify the webidl and ipdl root directories. r=mshal
https://hg.mozilla.org/integration/autoland/rev/dc8da8c97602
Populate WebIDLCollection from the emitter rather than the common backend. r=mshal
https://hg.mozilla.org/integration/autoland/rev/6001f7618569
Populate IPDLCollection from the emitter rather than the common backend. r=mshal
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.