Could you be more specific? IPDL already doesn't rewrite the IPDL files unless they have changed, even though it regenerates them internally on every export pass? Are you asking that IPDL be taught to calculate dependencies and avoid even the internal rebuild? That's hard, and might not be worth it.

To expand on comment 1, IPDL internally compiles everything on every invocation to avoid regenerating .h's/.cpp's if no .ipdl's changed. Changing some of the .h's (like PContent*.h) can trigger rebuilding a large portion of the tree. So far we've preferred the more precise dependency tracking provided by this scheme (more precise as compared to timestamp tracking) because the IPDL compiler itself isn't slow ... usually. In "normal" python environments the IPDL compiler goes through the .ipdl's at ~10/second. The only real issue I've seen is in scratchbox environments, which have a ridiculously slow python binary. I certainly don't want to change this compilation scheme just to work around scratchbox borkenness.

I have never seen ipdl files go by at 10/second. It's closer to 2-3/second on my local machine.

very annoying problem... make every build_all much longer

(In reply to comment #2) > The only real issue I've seen is in scratchbox environments, which > have a ridiculously slow python binary. I certainly don't want to change this > compilation scheme just to work around scratchbox borkenness. I should say, *something* about scratchbox makes the IPDL ridiculously slow. I don't know that it's the python binary itself.

Ok, I see that in scratchbox by some reason we are using system installed python, /scratchbox/tools/bin/misc_runner /usr/bin/python2.6 /usr/bin/python2.6 /home/romaxa/mozilla-central/config/pythonpath.py...... Scratchbox by default has only Python 2.3.4 (#1, Nov 8 2010, 09:32:39)

ah, and main reason is that we are running arm python with Qemu emulation, that is the reason why it so slow.

On my two year old Linux64 box the .ipdl files take about 10 seconds to generate. They are re-generated even if I haven't touched a single file. Multiply that by the number of browser builds done by developers per year, etc, etc. > Are you asking that IPDL be taught to calculate dependencies and avoid > even the internal rebuild? That's hard, and might not be worth it. The broader question is: are fast builds worth it?

See also bug 680534 and bug 680536.

(In reply to Ed Morley [:edmorley] from comment #9) > See also bug 680534 and bug 680536. Those aren't related to this. I'm not really working on build system enhancements right now, somebody could feel free to take this.

I might be wrong but it seems to be fixed...

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #11) > I might be wrong but it seems to be fixed... No, it's not. I just made it silent by default. ;-) (see bug 686507)

(In reply to Ehsan Akhgari [:ehsan] from comment #12) > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #11) > > I might be wrong but it seems to be fixed... > > No, it's not. I just made it silent by default. ;-) (see bug 686507) Oh... I would not have done that. Hiding an issue is very often the best way to not have it fixed :(

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #13) > (In reply to Ehsan Akhgari [:ehsan] from comment #12) > > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #11) > > > I might be wrong but it seems to be fixed... > > > > No, it's not. I just made it silent by default. ;-) (see bug 686507) > > Oh... I would not have done that. Hiding an issue is very often the best way > to not have it fixed :( Well, I didn't see anybody rushing to fix it, and it made me mad every time that I saw it in my nice silent console. So I took action, and did my best. If I knew how to fix this bug, I would've. ;-)

Speed up building ipc/. On my Mac it goes from 5 seconds to 0.6 seconds (x2 for universal). Bjacob got similar results on Linux. I'm sure in some settings the speed ups are even better.

Sorry for bug spam, forgot to qref

heh, on scartchbox this patch decrease build time up to couple of minutes...

Cool, I'm glad to hear that.

Comment on attachment 578129 [details] [diff] [review] Faster incremental builds for all the people! >diff --git a/ipc/ipdl/ipdl.py b/ipc/ipdl/ipdl.py >+op.add_option('-m', '--dependencies', dest='emmitdependencies', default=0, action='store_const', const=1, >+ help="Emmit Makefile dependencies for incremental rebuilds") > "emit" This wants to be a |default=False|, |action='store_true'| rule. >+ if emmitdependencies == 1: Nit: |if emitdependencies:| >diff --git a/ipc/ipdl/ipdl/__init__.py b/ipc/ipdl/ipdl/__init__.py > def genipdl(ast, outdir): >- return IPDLCodeGen().cgen(ast) >+ IPDLCodeGen(tempfile).cgen(ast) >+ I don't understand this change. I'm also not sure how it doesn't throw an undefined reference error. >+ >+def genm(ast, dir, filename): >+ tempfile = StringIO() >+ DependGen(tempfile).mgen(ast) >+ filename = dir + "/" + os.path.basename(filename) + ".depends" >+ print "Outputing to " + filename >+ writeifmodified(tempfile.getvalue(), filename) > You don't necessarily need to writeifmodified() the .depends file, since it's unconditionally read by the Makefile, and it might be faster anyway to directly overwrite the .depends. If you don't have evidence that either way is faster, I'd prefer to remove the writeifmodified(). > > def writeifmodified(contents, file): >+ # Bug 629668 >+ # We need to always rewrite modified files for incremental rebuilds >+ # to work correctly because makes wants all the file written to be >+ # never then EVERY ipdl. We could also touch the file when there >+ # are no changes but we don't really care about incremental builds >+ # when modifying IPDL files. If we're not meaningfully using writeifmodified() anymore (it looks like we aren't), let's just remove it. Nice patch :). Would like to see the next version.

Try run for 88f62e92a2f2 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=88f62e92a2f2 Results (out of 14 total builds): failure: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-88f62e92a2f2

Try run for 9ad184100042 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=9ad184100042 Results (out of 14 total builds): failure: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-9ad184100042

Try run for 121468338f95 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=121468338f95 Results (out of 145 total builds): exception: 1 success: 126 warnings: 11 failure: 7 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-121468338f95

I'm a bit confused by this error. Any python experts have an idea? I assume python has short circuit evaluation, thus we shouldn't call makedirs if the path already exists. Traceback (most recent call last): File "/builds/slave/try-osx64/build/config/pythonpath.py", line 52, in <module> main(sys.argv[1:]) File "/builds/slave/try-osx64/build/config/pythonpath.py", line 44, in main execfile(script, frozenglobals) File "/builds/slave/try-osx64/build/ipc/ipdl/ipdl.py", line 120, in <module> ipdl.gencxx(filename, ast, headersdir, cppdir) File "/builds/slave/try-osx64/build/ipc/ipdl/ipdl/__init__.py", line 76, in gencxx writetofile(tempfile.getvalue(), filename) File "/builds/slave/try-osx64/build/ipc/ipdl/ipdl/__init__.py", line 92, in writetofile os.path.exists(dir) or os.makedirs(dir) File "/tools/buildbot/bin/../lib/python2.6/os.py", line 157, in makedirs mkdir(name, mode) OSError: [Errno 17] File exists: '_ipdlheaders/mozilla/ipc'

(In reply to Benoit Girard (:BenWa) from comment #25) > I'm a bit confused by this error. Any python experts have an idea? > > I assume python has short circuit evaluation, thus we shouldn't call > makedirs if the path already exists. Correct. > Traceback (most recent call last): > File "/builds/slave/try-osx64/build/config/pythonpath.py", line 52, in > <module> > main(sys.argv[1:]) > File "/builds/slave/try-osx64/build/config/pythonpath.py", line 44, in main > execfile(script, frozenglobals) > File "/builds/slave/try-osx64/build/ipc/ipdl/ipdl.py", line 120, in > <module> > ipdl.gencxx(filename, ast, headersdir, cppdir) > File "/builds/slave/try-osx64/build/ipc/ipdl/ipdl/__init__.py", line 76, > in gencxx > writetofile(tempfile.getvalue(), filename) > File "/builds/slave/try-osx64/build/ipc/ipdl/ipdl/__init__.py", line 92, > in writetofile > os.path.exists(dir) or os.makedirs(dir) > File "/tools/buildbot/bin/../lib/python2.6/os.py", line 157, in makedirs > mkdir(name, mode) > OSError: [Errno 17] File exists: '_ipdlheaders/mozilla/ipc' Do we have a race condition here?

os.makedirs will raise an OSError if the target directory exists. It is recommended to protect calls to os.makedirs with os.path.exists. But, since we could have parallel execution, you will need to do something like: try: os.makedirs(path) except OSError as ex if ex.errno != errno.EEXIST: raise ex # else directory already exists. silently ignore and continue

Thanks for the suggestion, pushed to try.

Try run for 0afdb4d54914 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0afdb4d54914 Results (out of 14 total builds): failure: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-0afdb4d54914

Try run for 0afdb4d54914 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0afdb4d54914 Results (out of 14 total builds): failure: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-0afdb4d54914

Try run for 0afdb4d54914 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0afdb4d54914 Results (out of 14 total builds): failure: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-0afdb4d54914

Try run for 0afdb4d54914 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0afdb4d54914 Results (out of 14 total builds): failure: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-0afdb4d54914

Try run for 0afdb4d54914 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0afdb4d54914 Results (out of 14 total builds): failure: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-0afdb4d54914

Try run for 0afdb4d54914 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0afdb4d54914 Results (out of 14 total builds): failure: 14 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-0afdb4d54914

I fail. There should be a colon in the except line: try: os.makedirs(path) except OSError as ex: if ex.errno != errno.EEXIST: raise ex # else directory already exists. silently ignore and continue

I tried it locally first but nothing happened since I didn't touch a .ipdl file. I also had to import errno. It failed again because 'except XXX as ex:' appears to no be supported in our python version on tinderbox. *sigh*

You probably want |except XXX, ex:|

Try run is looking good. I should probably avoid Python for the next little while :)

"except as" was added in Python 2.6 (http://docs.python.org/release/2.6.7/reference/compound_stmts.html#except and http://docs.python.org/release/2.5.4/ref/try.html). I thought we had at least 2.6 everywhere. I guess not. Double sorry about the confusion. Perhaps someone can explain to me why we ship Python 2.7 as part of the Windows Mozilla Build installer yet have Python 2.5 on our build infrastructure. That makes no sense to me.

Because the build infrastructure is not on the latest version of Mozilla Build.

This patch replaced a few |writeifmodified| I forgot and changed os.makedirs(dir) to be within a try, see Comment 26, Comment 27.

Try run for b69dacdf36f1 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=b69dacdf36f1 Results (out of 208 total builds): success: 178 warnings: 27 failure: 3 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-b69dacdf36f1

Try run for 4d6f1fd12970 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=4d6f1fd12970 Results (out of 36 total builds): exception: 5 success: 12 warnings: 6 failure: 13 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-4d6f1fd12970

Try run for b69dacdf36f1 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=b69dacdf36f1 Results (out of 208 total builds): success: 178 warnings: 27 failure: 3 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-b69dacdf36f1

(In reply to Mozilla RelEng Bot from comment #44) my apologies, this comment is ever so slightly different than comment 42 so the bot posted. you should not see any more dupe posts from the releng bot

Whoops, pre-emptive FIXED, need to get back into inbound mode, after our spell in m-c :-)

Backed out until Bug 711549 is fixed. https://hg.mozilla.org/mozilla-central/rev/3d74a7c6d71b

I'm not sure what's going on in bug 711549 and I've got my hands tied with bug 598873. It would be great if someone can take a look.

Sorry I haven't been able to debug the pymake problem that caused this patch to land. We should really try to land this. According to Comment 15 a year ago it saved 4 seconds for any build (including a non chance build). We have more IPDL files today so I expect this number to be higher. Can anyone debug the pymake problem? We could even try relanding this if we have reason to believe pymake build has sufficiently changed that it might of fixed the problem.

Try uses pymake now so just push?

BenWa, I am not certain but I think this patch does not work anymore. I tried it 4 months ago and it failed because of something like we have several .ipdl files that include the same .ipdlh file, and then it pulls the dependency several times and make complains about it. I think it was something like that. Anyway, try the patch again with a clobber build and it might fail before getting to the pymake problem.

I think ipdlh didn't exist when I wrote this patch. Likely I only need to handle the AST node for them and emit them to the deps file.

We should try to land this once bug 868536 lands. Moving IPDLSRCS to moz.build also solves the problem of having ipc/ipdl/Makefile.in reference other parts of the tree. In the new world, we just put a bunch of IPDL_SOURCES in moz.build files and a unified .mk with all of them is written out for ipc/ipdl/Makefile.in to consume. No more VPATH abuse and multiple file inclusion. \o/

Updated to work with .ipdlh files. Maybe pymake fixes have fixed other issues in the interim.

Comment on attachment 774221 [details] [diff] [review] make ipdlc generate actual dependencies for its files Review of attachment 774221 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/ipdl/ipdl/mgen.py @@ +13,5 @@ > +# > +# The Original Code is mozilla.org code. > +# > +# Contributor(s): > +# Benoit Girard <bgirard@mozilla.com> We should remove this license block

Despite that one working just fine on my Linux box and reducing the time taken by a no-op make in ipc/ by 90%, it bombed on try. Here's one that seems to work as advertised--at least on my Linux box, we'll see about try: https://tbpl.mozilla.org/?tree=Try&rev=445dab9cf371

Comment on attachment 774292 [details] [diff] [review] make ipdlc generate actual dependencies for its files Seems to work OK on Windows. I assert that trivial changes have been made to BenWa's original patch WRT the python and so cjones's r+ still holds. Going to request glandium's approval on the Makefile.in changes, though.

Comment on attachment 774292 [details] [diff] [review] make ipdlc generate actual dependencies for its files Adding implicit r+ from cjones.

Comment on attachment 774292 [details] [diff] [review] make ipdlc generate actual dependencies for its files Never mind! This is not quite what we want.

OK, the .depends files being generated by the last patches were completely screwy. This patch does a much better job with the depends files, e.g. for BluetoothTypes.ipdlh: ../../ipc/ipdl/_ipdlheaders/mozilla/dom/bluetooth/BluetoothTypes.h: /home/froydnj/src/mozilla-central-official/dom/bluetooth/ipc/BluetoothTypes.ipdlh BluetoothTypes.cpp: /home/froydnj/src/mozilla-central-official/dom/bluetooth/ipc/BluetoothTypes.ipdlh and for PBluetooth.ipdl (somewhat cut down, but rest assured we do include all the relevant protocol files): ../../ipc/ipdl/_ipdlheaders/mozilla/dom/bluetooth/PBluetooth.h: /home/froydnj/src/mozilla-central-official/dom/bluetooth/ipc/PBluetooth.ipdl ...more... ../../ipc/ipdl/_ipdlheaders/mozilla/dom/bluetooth/PBluetoothParent.h: /home/froydnj/src/mozilla-central-official/dom/bluetooth/ipc/PBluetooth.ipdl ...more... ../../ipc/ipdl/_ipdlheaders/mozilla/dom/bluetooth/PBluetoothChild.h: /home/froydnj/src/mozilla-central-official/dom/bluetooth/ipc/PBluetooth.ipdl ...more... PBluetooth.cpp: /home/froydnj/src/mozilla-central-official/dom/bluetooth/ipc/PBluetooth.ipdl ...more... PBluetoothParent.cpp: /home/froydnj/src/mozilla-central-official/dom/bluetooth/ipc/PBluetooth.ipdl ...more... PBluetoothChild.cpp: /home/froydnj/src/mozilla-central-official/dom/bluetooth/ipc/PBluetooth.ipdl /home/froydnj/src/mozilla-central-official/dom/ipc/PBlob.ipdl ...more... The header dependencies are constructed so as to match those in the .pp files generated by GCC; ideally this would work the same on Windows, will have to check somehow. Comments welcome.

I'm not entirely sure this patch improves the situation; touching dom/bluetooth/ipc/BluetoothTypes.ipdlh triggers a rebuild of all the object files anyway. It looks like this is due to the headers (e.g. PContent{Parent,Child}.cpp depend on PBluetooth.h, and I suspect lots of things depend on PContent*.h), but it might be more complicated. The patch *does* make no-op builds for ipc/ipdl significantly faster, so maybe that's enough of a win.

Comment on attachment 774418 [details] [diff] [review] make ipdlc generate actual dependencies for its files Review of attachment 774418 [details] [diff] [review]: ----------------------------------------------------------------- You need to write out an empty rule for each file that's listed as a dependency. e.g. BluetoothTypes.h: BluetoothInput1.ipdl BluetootInput1.ipdl: This is needed because if you delete one of the input files (happens a lot when refactor and rename files), make is like "I need to ensure BluetoothInput1.ipdl is up to date." But, if there is no BluetoothInput1.ipdl file and no target to make it exist, make is like "I don't know how to make target BluetoothInput1.ipdl" and aborts. You work around this by defining an empty rule.

Comment on attachment 774418 [details] [diff] [review] make ipdlc generate actual dependencies for its files Review of attachment 774418 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/ipdl/Makefile.in @@ +40,5 @@ > # which is why we don't have explicit .h/.cpp targets here > +export:: $(CPPSRCS) > + > +# XXX: we really need to list more python deps here > +$(CPPSRCS) : $(ALL_IPDLSRCS) $(srcdir)/ipdl.py This is why touching 1 file will rebuild all the outputs. This rule is saying "every file in CPPSRCS depends on every file in ALL_IPDLSRCS." There are ways to do this properly. I'm content with handling it in a follow-up bug: what you have is already a win for no-op builds. We'll fix it by the end of Q3 because this is on the build config quarterly goals list :)

Comment on attachment 774418 [details] [diff] [review] make ipdlc generate actual dependencies for its files Review of attachment 774418 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/ipdl/Makefile.in @@ +40,5 @@ > # which is why we don't have explicit .h/.cpp targets here > +export:: $(CPPSRCS) > + > +# XXX: we really need to list more python deps here > +$(CPPSRCS) : $(ALL_IPDLSRCS) $(srcdir)/ipdl.py In fact, I think you can just remove this dep on ALL_IPDLSRCS and use that variable instead of $^ in the command (which removes the need for filter-out). This should work fine because if the cpp files don't exist, they will be generated. If they do exist, the dependencies are in the dependencies file. BTW, this rule is going to be executed several times in parallel when doing make -j<n> because make is going to try to build several of the CPPSRCS in parallel, running the command for each of them. ::: ipc/ipdl/ipdl/__init__.py @@ +35,5 @@ > it is not.''' > return TypeCheck().check(ast, errout) > > > +def gencxx(emitdependencies, ipdlfilename, hlast, outheadersdir, outcppdir): the emitdependencies argument would feel better last with a default value. @@ +69,3 @@ > > + if emitdependencies: > + filename = outcppdir + "/" + os.path.basename(ipdlfilename) + ".depends" It would be better to place them next to other dependency files, with the same kind of name (.deps/file.ext.pp) @@ +73,5 @@ > > def genipdl(ast, outdir): > return IPDLCodeGen().cgen(ast) > > +def writetofile(contents, file): You're removing the single most important feature of this function that is to not write anything if the file exists and has the same content as what would be written. (That being said, we have a class to handle that: mozbuild.util.FileAvoidWrite) That's another reason why touching one file would rebuild everything. ::: ipc/ipdl/ipdl/mgen.py @@ +4,5 @@ > + > +import sys > + > +from ipdl.cgen import CodePrinter > +from ipdl.cxx.ast import TypeArray, Visitor You're not using TypeArray.

Comment on attachment 774418 [details] [diff] [review] make ipdlc generate actual dependencies for its files Review of attachment 774418 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/ipdl/ipdl/__init__.py @@ +73,5 @@ > > def genipdl(ast, outdir): > return IPDLCodeGen().cgen(ast) > > +def writetofile(contents, file): I didn't think it would be needed if we had proper dependencies. But then again we can save some time in the case that a dependency changed but it had no impact.

Comment on attachment 774418 [details] [diff] [review] make ipdlc generate actual dependencies for its files Review of attachment 774418 [details] [diff] [review]: ----------------------------------------------------------------- This looks generally fine but I'm going to wait to see the final patch you work out with gps and glandium. If they're happy I probably will be too :) > BluetoothTypes.cpp: /home/froydnj/src/mozilla-central-official/dom/bluetooth/ipc/BluetoothTypes.ipdlh Do our dependencies normally have absolute paths like this?

Current patch. Now doesn't regenerate cpps at all when an ipdl file is changed. Also still has the issue that parallel makes run ipdl.py multiple times, which is highly suboptimal. Need to investigate running ipdl.py for individual .ipdl files and a separate invocation for IPCMessageStart.h.

(In reply to Nathan Froyd (:froydnj) (AFK until 22 July) from comment #70) > Need to investigate running ipdl.py for individual .ipdl > files and a separate invocation for IPCMessageStart.h. I am having problems making this approach work. Declaring the dependencies when ipdlsrcs.mk is created is simple enough: %.cpp %.h: .../%.ipdlh $(call run-ipdlc,$<) (defined similarly for .ipdl files and all the files they generate). This idiom avoids issues with running ipdl.py for every target in parallel make. It does have the downside of making clobber builds--and maybe even small change builds--slower. (This is because even compiling simple .ipdlh files tends to require parsing a good chunk of all the .ipdl files in the tree, so you're repeating the parsing work across N files now...) Anyway, solutions to collect and use proper dependency information and make parallel makes not do insane amounts of work and not slow down the clobber/serial build case work correctly welcome. (Pick any two?) (I'm also not sure this GNU make idiom works correctly in pymake.) But ipdl.py seems to have issues when processing single files and/or processing files in an order other than the one we use. For instance, running: ipdl.py <necessary options> /path/to/DOMTypes.ipdlh will fail with: /home/froydnj/src/mozilla-central-official.git/dom/ipc/DOMTypes.ipdlh:0: error: field `optionalInputStreamParams' of struct `ParentBlobConstructorParams' has unknown type `OptionalInputStreamParams' OptionalInputStreamParams is defined in ipc/glue/InputStreamParams.ipdlh. However: ipdl.py <necessary options> /path/to/DOMTypes.ipdlh /path/to/InputStreamParams.ipdlh fails the same way. But reversing the order: ipdl.py <necessary options> /path/to/InputStreamParams.ipdlh /path/to/DOMTypes.ipdlh works just fine. (This last one is implicitly the order we use, btw). Chris, do you have any insight into why the type-checking of these files would depend on the order in which we process the files?

I do not recommend doing any of this in make. We are trying to move all of the export tier (which includes the IPDL compilation step) away from make entirely. Instead we should continue to run a single ipdl.py command line and build the dependency system into the IPDL compiler itself.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #72) > I do not recommend doing any of this in make. We are trying to move all of > the export tier (which includes the IPDL compilation step) away from make > entirely. Instead we should continue to run a single ipdl.py command line > and build the dependency system into the IPDL compiler itself. That'd be interesting. So in today's world we'd keep the current |export:| invocation of ipdl.py, but make ipdl.py only write files if their dependencies had changed? Isn't this essentially what the writeifmodified bits do? Or am I misunderstanding? I don't know that any changes in this area are worthwhile under the Big Ball of Yarn that is the web of the .ipdl dependency graph gets untangled somewhat.

> That'd be interesting. So in today's world we'd keep the current |export:| > invocation of ipdl.py, but make ipdl.py only write files if their > dependencies had changed? > > Isn't this essentially what the writeifmodified bits do? Or am I > misunderstanding? That is what writeifmodified does, but it's not what I'm proposing exactly. I'm proposing an additional step of not *compiling* the IPDL files if any of the inputs hasn't changed. Even if we can just avoid recompiling any IPDL if *no* IPDL changes were made (which is the common case for depend builds) that would help a lot.

Ah, OK; this patch implements timestamp checking of all the inputs vs. all the outputs to determine if any work is necessary. Feels a little hackish, but definitely speeds up no-op builds on my machine (x86-64 Linux @ 2.66 GHz). Before: [froydnj@cerebro ipdl]$ time make -srj20 real 0m7.927s user 0m7.748s sys 0m0.144s After: [froydnj@cerebro ipdl]$ time make -srj20 real 0m0.542s user 0m0.508s sys 0m0.016s I've flagged Ben for review, but if any of the python-wise folks want to jump in with comments, I'd appreciate those too.

Comment on attachment 779307 [details] [diff] [review] make ipdl.py check timestamps to determine whether parsing/codegen is necessary Review of attachment 779307 [details] [diff] [review]: ----------------------------------------------------------------- This is a nice workaround. It would be nice to avoid the ipdl.py invocation if nothing has changed. But at 0.5s overhead (compared to 8), I'll take what you have here and we can optimize the Makefiles in a followup bug.

Comment on attachment 779307 [details] [diff] [review] make ipdl.py check timestamps to determine whether parsing/codegen is necessary Review of attachment 779307 [details] [diff] [review]: ----------------------------------------------------------------- r=me with this addressed: ::: ipc/ipdl/ipdl.py @@ +60,5 @@ > +headersmap = {} > +for (path, dirs, headers) in os.walk(headersdir): > + for h in headers: > + base = os.path.basename(h) > + headersmap[base] = os.path.join(path, h) This will cause problems if we ever have two protocols with the same name in different namespaces... Could you abort and print a message here if headersmap[base] has something in it already?

(In reply to ben turner [:bent] from comment #77) > Comment on attachment 779307 [details] [diff] [review] > make ipdl.py check timestamps to determine whether parsing/codegen is > necessary > > Review of attachment 779307 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with this addressed: > > ::: ipc/ipdl/ipdl.py > @@ +60,5 @@ > > +headersmap = {} > > +for (path, dirs, headers) in os.walk(headersdir): > > + for h in headers: > > + base = os.path.basename(h) > > + headersmap[base] = os.path.join(path, h) > > This will cause problems if we ever have two protocols with the same name in > different namespaces... Could you abort and print a message here if > headersmap[base] has something in it already? Good point. Will address with: base = os.path.basename(h) if base in root, ext = os.path.splitext(base) print >>sys.stderr, 'A protocol named', root, 'exists in multiple namespaces' sys.exit(1) headersmap[base] = os.path.join(path, h) (In reply to Gregory Szorc [:gps] from comment #76) > This is a nice workaround. It would be nice to avoid the ipdl.py invocation > if nothing has changed. But at 0.5s overhead (compared to 8), I'll take what > you have here and we can optimize the Makefiles in a followup bug. It's worth noting that the 0.5s overhead is for the entirety of |make|; a no-op |make export| is about 0.05s with this patch.

Few days (or month) back I see Google Chrome dev landing something similar bug fix in their Chromium snapshot, I bookmarked one of the relevant study link they did to speed up Blink IDL Compiler. Have a look at it, might be able to find some good information: https://docs.google.com/document/d/1AnPi-hprU7wiSd1jz4eyP5nPQgtLnFCalHC7kMJ9lXI/edit# Don't mind my stupid comment...

(In reply to Benjamin Smedberg [:bsmedberg] from comment #72) > I do not recommend doing any of this in make. We are trying to move all of > the export tier (which includes the IPDL compilation step) away from make > entirely. Instead we should continue to run a single ipdl.py command line > and build the dependency system into the IPDL compiler itself. I don't understand why we wouldn't still generate the .deps file. This will avoid invoking the compiler at all and works well with our current setup. We shouldn't be forking any processes if there's nothing to do. I'd much prefer seeing the build system understand the dependencies better. We can still have all the IPDL targets depend on all the IPDL source + deps and be built as the same target and benefit from the work done here.

(In reply to Benoit Girard (:BenWa) from comment #80) > I don't understand why we wouldn't still generate the .deps file. This will > avoid invoking the compiler at all and works well with our current setup. We > shouldn't be forking any processes if there's nothing to do. I'd much prefer > seeing the build system understand the dependencies better. We can still > have all the IPDL targets depend on all the IPDL source + deps and be built > as the same target and benefit from the work done here. I hear this, but the problem so far has been: 1) |export| should depend on the .cpp/.h files. 2) The .cpp/.h files should depend on the appropriate IPDL file. But then what do you write for the rules to regenerate the individual .cpp/.h files? You shouldn't compile all the IPDL files for each set of files, because then you're invoking the IPDL compiler many many times for no real benefit. And you can't say: export:: $(CPPFILES) $(CPPFILES) : ... $(PYTHON) $(srcdir)/ipdl.py ... with deps for the individual CPPFILES because that also invokes the compiler many many times for no benefit in parallel makes. (*maybe* you can do something clever with pattern rules on the left side, but I have no idea whether pymake supports that.) You can't compile individual IPDL files because something in the typechecker doesn't work right (arguably a bug and should be fixed). So dependency-driven recompilation doesn't work as things currently stand. Checking for "no work needs to be done" is simple enough and doesn't require Makefile contortions to make it work for all cases. Furthermore, the dependencies between the IPDL files themselves are so tangled that compiling a single file virtually requires loading all the IPDL files anyway. I've attached a PNG of a graph of the dependencies between the IPDL files. X -> Y indicates that X includes Y. Note the prevalence of cycles. The cycle-free outliers are in bluetooth and plugins, but I'm pretty sure nearly all of the graph is reachable from an arbitrary node.

I agree with everything you've said above. I think the IPDL rule should just be: $(ALL_CPPS) :(or ::?) $(ALL_IPDL_AND_DEPENDENCY) $(PYTHON) $(srcdir)/ipdl.py ... Thus we're telling Make to not run IPDL if no dependencies have changed. And we're telling make that if any IPDL dependencies have changed a single run of ipdl.py will update all the required targets. Then your patch above kicks in and only does work for the targets that changed. We get: - Make understands the dependencies and doesn't have to run ipdl.py needlessly - ipdl.py will compile all files that have changed at once but no more thanks to your patch.

(In reply to Benoit Girard (:BenWa) from comment #82) > I agree with everything you've said above. I think the IPDL rule should just > be: > > $(ALL_CPPS) :(or ::?) $(ALL_IPDL_AND_DEPENDENCY) > $(PYTHON) $(srcdir)/ipdl.py ... > > Thus we're telling Make to not run IPDL if no dependencies have changed. And > we're telling make that if any IPDL dependencies have changed a single run > of ipdl.py will update all the required targets. What you are writing in English is not the same thing you are writing in Makefile-ese. When you write: a.cpp b.cpp c.cpp: $(DEPS) do-something... that is interpreted by make as: a.cpp: $(DEPS) do-something... b.cpp: $(DEPS) do-something... c.cpp: $(DEPS) do-something... so if you |make -j3|, you are running |do-something| multiple times. That is suboptimal--especially if |do-something| rebuilds all three files anyway. It is particularly suboptimal with all the IPDL files we have and |do-something| being ipdl.py. I suppose you could argue that even with the multiple rules for rebuilding .cpp/.h files, in conjunction with the timestamp-checking in Python, then *some* of the runs of ipdl.py will exit quickly and not do any work. I am unsure that it would perform well with parallel builds, though; I could easily forsee a slowdown of N, where N is the # of parallel jobs you are running. The build system racing like this, even correctly, does not fill me with happiness. There is not a great way to tell make "hey, if *any* of these files are out of date, you just need to run a *single* command and that will update all of them". I invite you to read through: http://www.gnu.org/software/automake/manual/html_node/Multiple-Outputs.html for how to write a robust solution to this. I think, however, that the build peers would shoot me (or I would shoot myself) if I wrote a patch to implement this scheme.

No particular insight, sounds like a bug.

Thanks everyone for work on this bug. I understand there are still lingering concerns that things aren't super optimal. However, we did go from ~8s to ~0.5. Unless you are a build peer, I wouldn't worry too much about that remaining 0.5s. It will be addressed in due time, likely sometime this quarter as part of refactoring the export tier (bug 892644). I think the non-build folks have put in enough effort with make files and I encourage you to take a break and return to comfortable territory :)

I think this recently regressed. IPDLs are now taking about ~11s to build: $ time make ipdl /Users/gps/src/firefox/obj-firefox.noindex/_virtualenv/bin/python /Users/gps/src/firefox/config/pythonpath.py \ -I/Users/gps/src/firefox/other-licenses/ply \ /Users/gps/src/firefox/ipc/ipdl/ipdl.py -v \ --outheaders-dir=_ipdlheaders \ --outcpp-dir=. \ -I/Users/gps/src/firefox/content/media/webspeech/synth/ipc -I/Users/gps/src/firefox/dom/bluetooth/ipc -I/Users/gps/src/firefox/dom/devicestorage -I/Users/gps/src/firefox/dom/indexedDB/ipc -I/Users/gps/src/firefox/dom/ipc -I/Users/gps/src/firefox/dom/mobilemessage/src/ipc -I/Users/gps/src/firefox/dom/network/src -I/Users/gps/src/firefox/dom/plugins/ipc -I/Users/gps/src/firefox/dom/src/storage -I/Users/gps/src/firefox/gfx/layers/ipc -I/Users/gps/src/firefox/hal/sandbox -I/Users/gps/src/firefox/ipc/glue -I/Users/gps/src/firefox/ipc/testshell -I/Users/gps/src/firefox/js/ipc -I/Users/gps/src/firefox/layout/ipc -I/Users/gps/src/firefox/netwerk/cookie -I/Users/gps/src/firefox/netwerk/ipc -I/Users/gps/src/firefox/netwerk/protocol/ftp -I/Users/gps/src/firefox/netwerk/protocol/http -I/Users/gps/src/firefox/netwerk/protocol/websocket -I/Users/gps/src/firefox/netwerk/protocol/wyciwyg -I/Users/gps/src/firefox/uriloader/exthandler -I/Users/gps/src/firefox/uriloader/prefetch \ /Users/gps/src/firefox/content/media/webspeech/synth/ipc/PSpeechSynthesis.ipdl /Users/gps/src/firefox/content/media/webspeech/synth/ipc/PSpeechSynthesisRequest.ipdl /Users/gps/src/firefox/dom/bluetooth/ipc/BluetoothTypes.ipdlh /Users/gps/src/firefox/dom/bluetooth/ipc/PBluetooth.ipdl /Users/gps/src/firefox/dom/bluetooth/ipc/PBluetoothRequest.ipdl /Users/gps/src/firefox/dom/devicestorage/PDeviceStorageRequest.ipdl /Users/gps/src/firefox/dom/indexedDB/ipc/IndexedDBParams.ipdlh /Users/gps/src/firefox/dom/indexedDB/ipc/PIndexedDB.ipdl /Users/gps/src/firefox/dom/indexedDB/ipc/PIndexedDBCursor.ipdl /Users/gps/src/firefox/dom/indexedDB/ipc/PIndexedDBDatabase.ipdl /Users/gps/src/firefox/dom/indexedDB/ipc/PIndexedDBDeleteDatabaseRequest.ipdl /Users/gps/src/firefox/dom/indexedDB/ipc/PIndexedDBIndex.ipdl /Users/gps/src/firefox/dom/indexedDB/ipc/PIndexedDBObjectStore.ipdl /Users/gps/src/firefox/dom/indexedDB/ipc/PIndexedDBRequest.ipdl /Users/gps/src/firefox/dom/indexedDB/ipc/PIndexedDBTransaction.ipdl /Users/gps/src/firefox/dom/ipc/DOMTypes.ipdlh /Users/gps/src/firefox/dom/ipc/PBlob.ipdl /Users/gps/src/firefox/dom/ipc/PBlobStream.ipdl /Users/gps/src/firefox/dom/ipc/PBrowser.ipdl /Users/gps/src/firefox/dom/ipc/PContent.ipdl /Users/gps/src/firefox/dom/ipc/PContentDialog.ipdl /Users/gps/src/firefox/dom/ipc/PContentPermissionRequest.ipdl /Users/gps/src/firefox/dom/ipc/PCrashReporter.ipdl /Users/gps/src/firefox/dom/ipc/PDocumentRenderer.ipdl /Users/gps/src/firefox/dom/ipc/PMemoryReportRequest.ipdl /Users/gps/src/firefox/dom/ipc/PTabContext.ipdlh /Users/gps/src/firefox/dom/mobilemessage/src/ipc/PMobileMessageCursor.ipdl /Users/gps/src/firefox/dom/mobilemessage/src/ipc/PSms.ipdl /Users/gps/src/firefox/dom/mobilemessage/src/ipc/PSmsRequest.ipdl /Users/gps/src/firefox/dom/mobilemessage/src/ipc/SmsTypes.ipdlh /Users/gps/src/firefox/dom/network/src/PTCPServerSocket.ipdl /Users/gps/src/firefox/dom/network/src/PTCPSocket.ipdl /Users/gps/src/firefox/dom/plugins/ipc/PBrowserStream.ipdl /Users/gps/src/firefox/dom/plugins/ipc/PPluginBackgroundDestroyer.ipdl /Users/gps/src/firefox/dom/plugins/ipc/PPluginIdentifier.ipdl /Users/gps/src/firefox/dom/plugins/ipc/PPluginInstance.ipdl /Users/gps/src/firefox/dom/plugins/ipc/PPluginModule.ipdl /Users/gps/src/firefox/dom/plugins/ipc/PPluginScriptableObject.ipdl /Users/gps/src/firefox/dom/plugins/ipc/PPluginStream.ipdl /Users/gps/src/firefox/dom/plugins/ipc/PPluginSurface.ipdl /Users/gps/src/firefox/dom/plugins/ipc/PStreamNotify.ipdl /Users/gps/src/firefox/dom/src/storage/PStorage.ipdl /Users/gps/src/firefox/gfx/layers/ipc/LayerTransaction.ipdlh /Users/gps/src/firefox/gfx/layers/ipc/LayersSurfaces.ipdlh /Users/gps/src/firefox/gfx/layers/ipc/PCompositable.ipdl /Users/gps/src/firefox/gfx/layers/ipc/PCompositor.ipdl /Users/gps/src/firefox/gfx/layers/ipc/PGrallocBuffer.ipdl /Users/gps/src/firefox/gfx/layers/ipc/PImageBridge.ipdl /Users/gps/src/firefox/gfx/layers/ipc/PLayer.ipdl /Users/gps/src/firefox/gfx/layers/ipc/PLayerTransaction.ipdl /Users/gps/src/firefox/hal/sandbox/PHal.ipdl /Users/gps/src/firefox/ipc/glue/InputStreamParams.ipdlh /Users/gps/src/firefox/ipc/glue/URIParams.ipdlh /Users/gps/src/firefox/ipc/testshell/PTestShell.ipdl /Users/gps/src/firefox/ipc/testshell/PTestShellCommand.ipdl /Users/gps/src/firefox/js/ipc/JavaScriptTypes.ipdlh /Users/gps/src/firefox/js/ipc/PJavaScript.ipdl /Users/gps/src/firefox/layout/ipc/PRenderFrame.ipdl /Users/gps/src/firefox/netwerk/cookie/PCookieService.ipdl /Users/gps/src/firefox/netwerk/ipc/NeckoChannelParams.ipdlh /Users/gps/src/firefox/netwerk/ipc/PNecko.ipdl /Users/gps/src/firefox/netwerk/ipc/PRemoteOpenFile.ipdl /Users/gps/src/firefox/netwerk/protocol/ftp/PFTPChannel.ipdl /Users/gps/src/firefox/netwerk/protocol/http/PHttpChannel.ipdl /Users/gps/src/firefox/netwerk/protocol/websocket/PWebSocket.ipdl /Users/gps/src/firefox/netwerk/protocol/wyciwyg/PWyciwygChannel.ipdl /Users/gps/src/firefox/uriloader/exthandler/PExternalHelperApp.ipdl /Users/gps/src/firefox/uriloader/prefetch/POfflineCacheUpdate.ipdl Generated C++ headers will be generated relative to "_ipdlheaders" Generated C++ sources will be generated in "." PSpeechSynthesis.ipdl checking types PSpeechSynthesisRequest.ipdl checking types BluetoothTypes.ipdlh checking types PBluetooth.ipdl checking types PBluetoothRequest.ipdl checking types PDeviceStorageRequest.ipdl checking types IndexedDBParams.ipdlh checking types PIndexedDB.ipdl checking types PIndexedDBCursor.ipdl checking types PIndexedDBDatabase.ipdl checking types PIndexedDBDeleteDatabaseRequest.ipdl checking types PIndexedDBIndex.ipdl checking types PIndexedDBObjectStore.ipdl checking types PIndexedDBRequest.ipdl checking types PIndexedDBTransaction.ipdl checking types DOMTypes.ipdlh checking types PBlob.ipdl checking types PBlobStream.ipdl checking types PBrowser.ipdl checking types PContent.ipdl checking types PContentDialog.ipdl checking types PContentPermissionRequest.ipdl checking types PCrashReporter.ipdl checking types PDocumentRenderer.ipdl checking types PMemoryReportRequest.ipdl checking types PTabContext.ipdlh checking types PMobileMessageCursor.ipdl checking types PSms.ipdl checking types PSmsRequest.ipdl checking types SmsTypes.ipdlh checking types PTCPServerSocket.ipdl checking types PTCPSocket.ipdl checking types PBrowserStream.ipdl checking types PPluginBackgroundDestroyer.ipdl checking types PPluginIdentifier.ipdl checking types PPluginInstance.ipdl checking types PPluginModule.ipdl checking types PPluginScriptableObject.ipdl checking types PPluginStream.ipdl checking types PPluginSurface.ipdl checking types PStreamNotify.ipdl checking types PStorage.ipdl checking types LayerTransaction.ipdlh checking types LayersSurfaces.ipdlh checking types PCompositable.ipdl checking types PCompositor.ipdl checking types PGrallocBuffer.ipdl checking types PImageBridge.ipdl checking types PLayer.ipdl checking types PLayerTransaction.ipdl checking types PHal.ipdl checking types InputStreamParams.ipdlh checking types URIParams.ipdlh checking types PTestShell.ipdl checking types PTestShellCommand.ipdl checking types JavaScriptTypes.ipdlh checking types PJavaScript.ipdl checking types PRenderFrame.ipdl checking types PCookieService.ipdl checking types NeckoChannelParams.ipdlh checking types PNecko.ipdl checking types PRemoteOpenFile.ipdl checking types PFTPChannel.ipdl checking types PHttpChannel.ipdl checking types PWebSocket.ipdl checking types PWyciwygChannel.ipdl checking types PExternalHelperApp.ipdl checking types POfflineCacheUpdate.ipdl checking types real 0m10.158s user 0m9.972s sys 0m0.175s I plan to just avoid the ipdl.py invocation altogether in bug 907394. But someone may want to look into why we're not spending 0.5s any more.

(In reply to Gregory Szorc [:gps] from comment #87) > I think this recently regressed. IPDLs are now taking about ~11s to build: > > real 0m10.158s > user 0m9.972s > sys 0m0.175s > > I plan to just avoid the ipdl.py invocation altogether in bug 907394. But > someone may want to look into why we're not spending 0.5s any more. Takes a half-second for me locally on an already built tree. You sure you weren't timing that on a clobber-ish build?

I used the 2nd time from a local run!