Bug 117708

Created attachment 204838 [details] Patch

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 ...

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;" ?

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.

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.

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

*** This bug has been marked as a duplicate of bug 76836 ***

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.

(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.

Created attachment 204885 [details] Alternative patch

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.

(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] :(

Comment on attachment 204885 [details] Alternative patch Clearing flags on attachment: 204885 Committed r151675: <http://trac.webkit.org/changeset/151675>

All reviewed patches have been landed. Closing bug.

After touching an idl without making functional changes (such as editing a comment), the preprocessor still runs every time you compile.

(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).

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.

(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?

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.

I put my proposed alternate fix in bug 117900, let me know if there is a reason it's wrong.

(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?

> 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.