Bug 77510 - In AppleWebKit and AppleWin, stop rebuilding IDLs that need not to be rebuilt
Summary: In AppleWebKit and AppleWin, stop rebuilding IDLs that need not to be rebuilt
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 76970
  Show dependency treegraph
 
Reported: 2012-01-31 21:52 PST by Kentaro Hara
Modified: 2012-02-07 01:01 PST (History)
5 users (show)

See Also:


Attachments
WIP patch (10.92 KB, patch)
2012-02-01 05:03 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
WIP patch (11.75 KB, patch)
2012-02-01 06:01 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (22.25 KB, patch)
2012-02-06 03:07 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-01-31 21:52:17 PST
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.
Comment 1 Kentaro Hara 2012-02-01 05:03:48 PST
Created attachment 124931 [details]
WIP patch
Comment 2 Kentaro Hara 2012-02-01 05:07:30 PST
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.
Comment 3 Kentaro Hara 2012-02-01 06:01:15 PST
Created attachment 124938 [details]
WIP patch

Fixed the infinite loop issue by handling JavaScriptCallFrame.idl specially
Comment 4 Darin Adler 2012-02-01 07:51:38 PST
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 "<".
Comment 5 Kentaro Hara 2012-02-01 07:56:18 PST
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?
Comment 6 Kentaro Hara 2012-02-03 03:44:57 PST
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.
Comment 7 Darin Adler 2012-02-03 11:31:57 PST
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.
Comment 8 Kentaro Hara 2012-02-06 03:07:41 PST
Created attachment 125607 [details]
Patch
Comment 9 Kentaro Hara 2012-02-06 03:12:59 PST
(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?)
Comment 10 Adam Barth 2012-02-06 13:33:03 PST
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.
Comment 11 WebKit Review Bot 2012-02-06 15:11:15 PST
Comment on attachment 125607 [details]
Patch

Clearing flags on attachment: 125607

Committed r106862: <http://trac.webkit.org/changeset/106862>
Comment 12 WebKit Review Bot 2012-02-06 15:11:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Kentaro Hara 2012-02-07 00:54:27 PST
Reverted r106862 for reason:

Mac build fails if we manually remove generated code

Committed r106913: <http://trac.webkit.org/changeset/106913>
Comment 14 Kentaro Hara 2012-02-07 01:01:27 PST
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.