Bug 77510 – In AppleWebKit and AppleWin, stop rebuilding IDLs that need not to be rebuilt

• Component: WebCore JavaScript
• Status: RESOLVED CONFIGURATION CHANGED

[Kentaro Hara]

Currently, if any IDL is updated, all IDLs are rebuilt. We should stop rebuilding IDLs which are not modified and whose supplemental dependency are not changed.

[Kentaro Hara]

Created attachment 124931 [details]
WIP patch

[Kentaro Hara]

Uploaded a WIP patch. The patch works fine in a clean build. However, when I touch Element.idl and try rebuild, makefile goes into infinite loop. This is strange since the dependency of makefile should be DAG. I've been debugging.

[Kentaro Hara]

Created attachment 124938 [details]
WIP patch

Fixed the infinite loop issue by handling JavaScriptCallFrame.idl specially

[Darin Adler]

Comment on attachment 124938 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124938&action=review

Looks like a good approach. The file name supplemental_dependency.tmp seems like it could be better. No reason the .tmp extension is particularly good. But I guess you already had that file name before this patch.

> Source/WebCore/bindings/scripts/update-idl-needs-rebuild.pl:68
> +    open FH, "> $file" or die "Couldn't open $file: $!";

Better syntax:

    open FH, ">", $file

The version where the ">" is in the filename string is old/obsolete. Same for "<".

[Kentaro Hara]

Adam, Darin: Thank you! Getting close to the goal, but one problem still remains. Even if I touch DOMWindowWebAudio.idl, DOMWindow.idl is not rebuilt. The reason is as follows.

The build flow:

> supplemental_dependency.tmp : $(ALL_IDLS)
>     perl resolve-supplemental.pl $(ALL_IDLS)
> 
> %.idl-needs-rebuild : %.idl | supplemental_dependency.tmp
>     touch %.idl-needs-rebuild
>     perl update-idl-needs-rebuild.pl $< $@ # update-idl-needs-rebuild.pl touches the "main" IDL's .idl-needs-rebuild if the .idl-needs-rebuild is older than %.idl
> 
> JS%.cpp : %.idl-needs-rebuild | *.idl-needs-rebuild
>     perl generate-bindings.pl supplemental_dependency.tmp $<    # Generates JS%.cpp


In particular, let us focus on this line:

> JSDOMWindow.cpp : DOMWindow.idl-needs-rebuild | *.idl-needs-rebuild
>     ...

At this point, the following five things can happen:

(a) All *.idl-needs-rebuild rules fire.
(b) The DOMWindow.idl-needs-rebuild rule fires.
(c) The timestamps of JSDOMWindow.cpp and DOMWindow.idl-needs-rebuild are compared.
(d) If JSDOMWindow.cpp is older than DOMWindow.idl-needs-rebuild, the JSDOMWindow.cpp rule is decided to fire.
(e) If the JSDOMWindow.cpp rule is decided to fire, the JSDOMWindow.cpp rule fires.

We were expecting that the following order would be guaranteed:
(a)->(c),  (b)->(c),  (c)->(d), (d)->(e)

However, actually what makefile guarantees is the following order:
(a)->(e),  (b)->(c),  (c)->(d), (d)->(e)

That way, if things happen in the order of (b)->(c)->(d)->(a)->(e), the JSDOMWindow.cpp rule does not fire. This actually happens in my experiments (even with -j 1).


You can confirm the behavior in the following simplified example:

all : A
A : B | C
	echo A
	touch A
B :
	echo B
C :
	sleep 2
	echo C
	touch B

If you run the makefile with -j 1, "echo A" will be output in the odd runs but will not be output in the even runs.

Any idea?

[Kentaro Hara]

Adam, Darin: dominicc@ suggested the following idea:

supplemental_dependency.tmp : $(ALL_IDLS)
    perl resolve-supplemental.pl ...

%.idl-needs-rebuild : %.idl supplemental_dependency.tmp
    perl update-idl-needs-rebuild.pl ...

JS%.cpp : %.idl-needs-rebuild
     perl generate-bindings.pl ...


resolve-supplemental.pl generates the following supplemental_dependency.tmp. The number next to a file name is the timestamp of the file.

A.idl(1200)
B.idl(1000) B-supplemental1.idl(800) B-supplemental2.idl(1200)
C.idl(1000)

update-idl-needs-rebuild.pl touches %.idl-needs-rebuild so that the timestamps of %.idl-needs-rebuild and %.idl become equal, if (1) %.idl-needs-rebuild is older than %.idl or (2) there is an IDL file (other than %.idl) whose timestamp is newer than %.idl in the %.idl's line of supplemental_dependency.tmp. For example, if the timestamps of A.idl-needs-rebuild, B.idl-needs-rebuild and C.idl-needs-rebuild are 1000, then A.idl-needs-rebuild and B.idl-needs-rebuild will be touched.

If any IDL is updated, resolve-supplemental.pl will run once, update-idl-needs-rebuild.pl will run for all IDL files, and generate-bindings.pl will run for the IDL files which are modified or whose dependencies are changed.

It appears it will work fine. The advantage of this idea is that it does not use makefile's tricky syntax. Thoughts? If you think this will work fine, I am happy to implement it.

[Darin Adler]

The only immediate problem I spot is that running the update-idl-needs-rebuild.pl script hundreds of times might be slow. Otherwise, sounds like it should be OK.

[Kentaro Hara]

Created attachment 125607 [details]
Patch

[Kentaro Hara]

(In reply to comment #7)
> The only immediate problem I spot is that running the update-idl-needs-rebuild.pl script hundreds of times might be slow. 

Darin, Adam: I confirmed that the uploaded patch works as we expect, except that update-idl-needs-rebuild.pl always runs for all IDL files even if no IDL files are updated. (If no IDLs are updated, update-idl-needs-rebuild.pl just runs and does nothing. It takes about 3 seconds in my local AppleWebKit. Would it be acceptable?)

[Adam Barth]

Comment on attachment 125607 [details]
Patch

IMHO, this is an improvement.  It's probably not the final solution.  We can continue to iterate in another patch.

[WebKit Review Bot]

Comment on attachment 125607 [details]
Patch

Clearing flags on attachment: 125607

Committed r106862: <http://trac.webkit.org/changeset/106862>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Reverted r106862 for reason:

Mac build fails if we manually remove generated code

Committed r106913: <http://trac.webkit.org/changeset/106913>

[Kentaro Hara]

Sadly I rolled out the patch because the patch will break AppleWebKit build.

> supplemental_dependency.tmp : $(ALL_IDLS)
>     perl resolve-supplemental.pl ...
> 
> %.idl-needs-rebuild : %.idl supplemental_dependency.tmp
>     perl update-idl-needs-rebuild.pl ...
> 
> JS%.cpp : %.idl-needs-rebuild
>      perl generate-bindings.pl ...

In the above build flow, if we manually remove JS%.cpp, the JS%.cpp will be never rebuilt again. (The same happens for JS%.h, WebDOM%.h, WebDOM%.cpp, etc) I tried several approaches to fix the issue, but could not find a sane approach.

[Fujii Hironori]

I think this problem is gone.