RESOLVED FIXED 117708
touching any idl rebuilds all derived sources
https://bugs.webkit.org/show_bug.cgi?id=117708
Summary touching any idl rebuilds all derived sources
Alex Christensen
Reported 2013-06-17 10:55:03 PDT
When any idl is changed, it rebuilds all 600 derived sources in WebCore. This slows down development for anyone who is working on derived sources.
Attachments
Patch (1.52 KB, patch)
2013-06-17 10:57 PDT, Alex Christensen
achristensen: review-
Alternative patch (4.64 KB, patch)
2013-06-18 00:06 PDT, Chris Dumez
no flags
Alex Christensen
Comment 1 2013-06-17 10:57:54 PDT
Alex Christensen
Comment 2 2013-06-17 11:04:55 PDT
This patch reads the contents of a file before writing them. This does not slow down a build after make clean because the files do not exist. One negative side effect of this is that after changing an idl, generate-bindings will be run each build (but not change anything) because of line 1022 of DerivedSources.make: JS%.h : %.idl ...
Chris Dumez
Comment 3 2013-06-17 11:17:49 PDT
Comment on attachment 204838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204838&action=review > Source/WebCore/bindings/scripts/CodeGenerator.pm:178 > + my $oldContents=join("",<FH>); nit: missing spaces around = and after comma > Source/WebCore/bindings/scripts/CodeGenerator.pm:180 > + if ($oldContents eq $contents) { maybe: "return if $oldContents eq $contents;" ?
Chris Dumez
Comment 4 2013-06-17 11:30:41 PDT
FYI, Blink uses the same optimization. However, it is only enabled with ninja with the following explanation: # The bindings generator can not write generated files if they are identical # to the already existing file – that way they don't need to be recompiled. # However, a reverse dependency having a newer timestamp than a # generated binding can confuse some build systems, so only use this on # ninja which explicitly supports this use case (gyp turns all actions into # ninja restat rules). I guess we could have a new script argument like --writeOnlyIfChanged in case we want to disable this for some build systems if we notice issues.
Alex Christensen
Comment 5 2013-06-17 11:34:38 PDT
If anyone commits a change to any idl file, everyone will be running generate-bindings on all idls until they make clean. We need a dependency generator for make instead of my jacky solution to prevent this.
Alex Christensen
Comment 6 2013-06-17 16:01:50 PDT
Maciej had this working in r120361 (https://bugs.webkit.org/show_bug.cgi?id=89125). I'll bisect over the next few days while working on other stuff and find out what broke his system
Alexey Proskuryakov
Comment 7 2013-06-17 16:10:58 PDT
*** This bug has been marked as a duplicate of bug 76836 ***
Maciej Stachowiak
Comment 8 2013-06-17 17:23:39 PDT
I don't think the duping here is correct. My original fix for this in bug 89125 did not involve examining whether .h/.cpp files had changed. Instead, my solution was to represent the supplemental IDL dependencies in Make format so that only affected IDL files would ever be rebuild. I am not sure why it broke. But fixing it is a separate issue from what 76836 recommends, which is that even if an IDL file does get recompiled by the IDL compiler, avoid running the C++ compiler.
Chris Dumez
Comment 9 2013-06-17 22:45:14 PDT
(In reply to comment #6) > Maciej had this working in r120361 (https://bugs.webkit.org/show_bug.cgi?id=89125). I'll bisect over the next few days while working on other stuff and find out what broke his system I am fairly certain I broke this recently when adding support for [NoInterfaceObject]. IDL inter-dependency is a bit more complex now and the build system integration likely needs some improvement. I will also look into this issue.
Chris Dumez
Comment 10 2013-06-18 00:06:31 PDT
Created attachment 204885 [details] Alternative patch
Kentaro Hara
Comment 11 2013-06-18 00:51:48 PDT
Comment on attachment 204885 [details] Alternative patch Good. The sad thing is that we cannot make the same optimization in GYP since GYP doesn't support dynamically generated dependency.
Kentaro Hara
Comment 12 2013-06-18 00:53:34 PDT
(In reply to comment #9) > (In reply to comment #6) > > Maciej had this working in r120361 (https://bugs.webkit.org/show_bug.cgi?id=89125). I'll bisect over the next few days while working on other stuff and find out what broke his system > > I am fairly certain I broke this recently when adding support for [NoInterfaceObject]. IDL inter-dependency is a bit more complex now and the build system integration likely needs some improvement. > I will also look into this issue. Yes, maciej@ fixed the issue before, but we regressed it when we introduced [NoInterfaceObject] :(
WebKit Commit Bot
Comment 13 2013-06-18 01:29:41 PDT
Comment on attachment 204885 [details] Alternative patch Clearing flags on attachment: 204885 Committed r151675: <http://trac.webkit.org/changeset/151675>
WebKit Commit Bot
Comment 14 2013-06-18 01:29:44 PDT
All reviewed patches have been landed. Closing bug.
Alex Christensen
Comment 15 2013-06-18 15:45:06 PDT
After touching an idl without making functional changes (such as editing a comment), the preprocessor still runs every time you compile.
Kentaro Hara
Comment 16 2013-06-18 16:54:58 PDT
(In reply to comment #15) > After touching an idl without making functional changes (such as editing a comment), the preprocessor still runs every time you compile. This is expected. To resolve dependency of 'partial', 'implements' and [NoInterfaceObject], we cannot avoid scanning all IDL files when any IDL file is touched. In my local experiment with SSD, the time spent in the preprocessor is very small (less than 1 sec).
Maciej Stachowiak
Comment 17 2013-06-21 16:54:46 PDT
Comment on attachment 204885 [details] Alternative patch The WriteFileIfChanged part makes me worry that this would leave you in a state where the supplemental deps list is regenerated every time. Does that in fact happen? I also don't get why touching supplemental_dependency.tmp is even relevant, since none of the IDL compilation rules actually depend on it.
Maciej Stachowiak
Comment 18 2013-06-21 16:56:21 PDT
(In reply to comment #16) > (In reply to comment #15) > > After touching an idl without making functional changes (such as editing a comment), the preprocessor still runs every time you compile. > > This is expected. To resolve dependency of 'partial', 'implements' and [NoInterfaceObject], we cannot avoid scanning all IDL files when any IDL file is touched. In my local experiment with SSD, the time spent in the preprocessor is very small (less than 1 sec). I don't see how this can be true for those features when it turned out not to be true for supplemental interfaces. How is partial different from supplements in that regard?
Maciej Stachowiak
Comment 19 2013-06-21 17:01:31 PDT
I think the real bug is this: JS%.h : %.idl $(JS_BINDINGS_SCRIPTS) $(IDL_ATTRIBUTES_FILE) $(WINDOW_CONSTRUCTORS_FILE) $(WORKERCONTEXT_CONSTRUCTORS_FILE) $(PLATFORM_FEATURE_DEFINES) $(WINDOW_CONSTRUCTORS_FILE) and $(WORKERCONTEXT_CONSTRUCTORS_FILE) are not passed to the generator script nor are the files they refer to referenced by it directly. So that decency is almost surely wrong. I think the correct fix is to revert this patch and remove those two things from the JS%.h dependencies.
Maciej Stachowiak
Comment 20 2013-06-21 17:34:55 PDT
I put my proposed alternate fix in bug 117900, let me know if there is a reason it's wrong.
Chris Dumez
Comment 21 2013-06-22 01:30:33 PDT
(In reply to comment #17) > (From update of attachment 204885 [details]) > The WriteFileIfChanged part makes me worry that this would leave you in a state where the supplemental deps list is regenerated every time. Does that in fact happen? I also don't get why touching supplemental_dependency.tmp is even relevant, since none of the IDL compilation rules actually depend on it. I tested with CMake and IDL compilation rule depends on supplemental_dependency.tmp in the CMake config. supplemental_dependency.tmp is passed to the generator so why shouldn't it be marked as a dependency?
Kentaro Hara
Comment 22 2013-06-22 06:25:04 PDT
> The WriteFileIfChanged part makes me worry that this would leave you in a state where the supplemental deps list is regenerated every time. Does that in fact happen? maciej: In my understanding, it in fact happens and it is expected. The point is as follows: - If any IDL file is touched, we have to scan all IDL files and resolve dependency of [NoInterfaceObject], 'partial' and 'implements'. Thus, it is expected that preprocessor-idls.pl always runs when any IDL file is touched. - However, we want to stop the build flow at this point if we find that there is no change in supplemental-dependency.tmp. We can do it by not updating supplemental-dependency.tmp in preprocessor-idls.pl.
Note You need to log in before you can comment on or make changes to this bug.