Bug 117708 – touching any idl rebuilds all derived sources

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 204838 [details]
Patch

[Alex Christensen]

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

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]

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]

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]

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

[Maciej Stachowiak]

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]

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

Created attachment 204885 [details]
Alternative patch

[Kentaro Hara]

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]

(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 on attachment 204885 [details]
Alternative patch

Clearing flags on attachment: 204885

Committed r151675: <http://trac.webkit.org/changeset/151675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

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

[Kentaro Hara]

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

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

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]

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

[Chris Dumez]

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

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