| Summary: | [meta] When any IDL is updated, all IDLs are rebuilt | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||
| Component: | WebCore JavaScript | Assignee: | Kentaro Hara <haraken> | ||||
| Status: | RESOLVED CONFIGURATION CHANGED | ||||||
| Severity: | Normal | CC: | abarth, darin, Hironori.Fujii, japhet, morrita, simon.fraser, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | 77510 | ||||||
| Bug Blocks: | 76836 | ||||||
| Attachments: |
|
||||||
|
Description
Kentaro Hara
2012-01-24 17:53:38 PST
This is what I was driving at when I asked how we would write the makefiles! I don’t remember what bug that comment was in. Created attachment 123863 [details]
Patch
(In reply to comment #1) > This is what I was driving at when I asked how we would write the makefiles! I don’t remember what bug that comment was in. Sorry, I noticed, now... Sounds like we need another plan to solve the original problem. Found it, <https://bugs.webkit.org/show_bug.cgi?id=74900#c20>. Comment on attachment 123863 [details] Patch Clearing flags on attachment: 123863 Committed r105844: <http://trac.webkit.org/changeset/105844> All reviewed patches have been landed. Closing bug. Adam, darin: This might be a hack (or might be still wrong:-), but how about the following idea?
==== Current behavior ====
supplemental_dependency.tmp : $(ALL_IDL_FILES)
perl resolve-supplemental.pl $(ALL_IDL_FILES) # resolve-supplemental.pl generates supplemental_dependency.tmp.
JS%.h : %.idl supplemental_dependency.tmp
perl generate-bindings.pl supplemental_dependency.tmp $< # generate-bindings.pl generates JS%.h and JS%.cpp.
==== New behavior ====
supplemental_dependency.tmp : $(ALL_IDL_FILES)
perl resolve-supplemental.pl $(ALL_IDL_FILES) # resolve-supplemental.pl generates supplemental_dependency.tmp.
supplemental_dependency_with_effective_timestamp.tmp : supplemental_dependency.tmp
cp ... # copy supplemental_dependency.tmp to supplemental_dependency_with_effective_timestamp.tmp only when the bytes differ.
JS%.h : %.idl supplemental_dependency_with_effective_timestamp.tmp
perl generate-bindings.pl supplemental_dependency_with_effective_timestamp.tmp $< # generate-bindings.pl generates JS%.h and JS%.cpp.
So, the copy rule will always fire, but we ought not worry because it will be fast? Isn't there a problem if I touch the contents of DOMWindowWebSockets.idl ? supplemental_dependency_with_effective_timestamp.tmp won't change but JSDOMWindow.cpp won't be re-built. (In reply to comment #9) > So, the copy rule will always fire, but we ought not worry because it will be fast? Yes. Some command must always fire anyway. So the basic idea is to make the command faster. (In reply to comment #10) > Isn't there a problem if I touch the contents of DOMWindowWebSockets.idl ? supplemental_dependency_with_effective_timestamp.tmp won't change but JSDOMWindow.cpp won't be re-built. Ah, right. More simple, a problem happens when we just touch Element.idl. supplemental_dependency_with_effective_timestamp.tmp won't change, and JSElement.cpp won't be re-built. The problem here is that supplemental_dependency_with_effective_timestamp.tmp is not updated when the update is required. Then how about adding a hash (e.g. MD5) of each IDL to supplemental_dependency.tmp, like this? A.idl (r843wqrdjwaoufe) B.idl (ru3oafjslajfe) B_supplemental.idl (fjdskafeiuwo32) B_Supplemental2.idl (r4owafnsfdlkre) C.idl (r483qokesafsda) Here's another idea; 1) Whenever any IDL file changes, run resolve-supplemental, and for each line in supplemental_dependency.tmp, if the main IDL file is older than any of the supplemental IDL files, touch the main IDL file. 2) Have JSFoo.cpp depend only on JSFoo.idl (the main IDL file). Thoughts? (In reply to comment #12) > Here's another idea; > > 1) Whenever any IDL file changes, run resolve-supplemental, and for each line in supplemental_dependency.tmp, if the main IDL file is older than any of the supplemental IDL files, touch the main IDL file. > 2) Have JSFoo.cpp depend only on JSFoo.idl (the main IDL file). > > Thoughts? Here’s another version of this that doesn’t rely on touching source files. Maybe not worded quite correctly: 1) For each main IDL file, have a make rule that creates/touches a corresponding .cpp-needs-to-be-gnerated file. 2) For each supplemental IDL file, have a make rule that createes/touches the main .cpp-needs-to-be-gnerated files as well as whatever file is generated to track the fact that the supplemental file was processed. This need not be done all at once with resolve-supplemental, or could be done for each supplemental file individually. There is no order dependency between this and (1). 3) In a separate pass (needs to be after [2], yet not dependent on [2]), have JSFoo.cpp depend on JSFoo.idl-needs-compliation. (In reply to comment #12) > Here's another idea; > > 1) Whenever any IDL file changes, run resolve-supplemental, and for each line in supplemental_dependency.tmp, if the main IDL file is older than any of the supplemental IDL files, touch the main IDL file. > 2) Have JSFoo.cpp depend only on JSFoo.idl (the main IDL file). > > Thoughts? Thanks! I might be misunderstanding, but you mean something like this? supplemental_dependency.tmp : $(ALL_IDL_FILES) perl resolve-supplemental.pl $(ALL_IDL_FILES) # resolve-supplemental.pl generates supplemental_dependency.tmp. It also touches main IDLs that must be updated. JS%.cpp : %.idl perl generate-bindings.pl supplemental_dependency.tmp $< # generate-bindings.pl generates JS%.h and JS%.cpp. In this case, the order dependency between generate-bindings.pl and resolve-supplemental.pl does not appear in the makefile. How can we confirm that generate-bindings.pl runs after resolve-supplemental.pl? # I am looking into darin's version. (In reply to comment #13) > (In reply to comment #12) > Here’s another version of this that doesn’t rely on touching source files. Maybe not worded quite correctly: > > 1) For each main IDL file, have a make rule that creates/touches a corresponding .cpp-needs-to-be-gnerated file. > > 2) For each supplemental IDL file, have a make rule that createes/touches the main .cpp-needs-to-be-gnerated files as well as whatever file is generated to track the fact that the supplemental file was processed. This need not be done all at once with resolve-supplemental, or could be done for each supplemental file individually. There is no order dependency between this and (1). > > 3) In a separate pass (needs to be after [2], yet not dependent on [2]), have JSFoo.cpp depend on JSFoo.idl-needs-compliation. Thanks, darin! I might be misunderstanding your idea, but you mean something like this? supplemental_dependency.tmp : $(ALL_IDL_FILES) perl resolve-supplemental.pl $(ALL_IDL_FILES) # Generates supplemental_dependency.tmp. %.cpp-needs-to-be-generated : %.idl supplemental_dependency.tmp perl touch-cpp-needs-to-be-generated.pl $< # touches %.cpp-needs-to-be-generated and all *.cpp-needs-to-be-generated s that %.idl is supplementing. JS%.cpp : %.cpp-needs-to-be-generated perl generate-bindings.pl supplemental_dependency.tmp $< # Generates JS%.h and JS%.cpp. In this case, how can we guarantee that generate-bindings.pl for JSxxxx.cpp runs after xxxx.cpp-needs-to-be-generated is touched by all touch-cpp-needs-to-be-generated.pl s that may touch xxxx.cpp-needs-to-be-generated? I guess that we need to guarantee that generate-bindings.pl for each IDL must run after all supplemental dependencies are resolved. This implies that JS%.cpp must (directly or indirectly) depend on a file that describes all the supplemental dependencies (i.e. supplemental_dependency.tmp) as a explicit rule in makefile, like this:
JS%.cpp : %.idl supplemental_dependency.tmp
perl generate-bindings.pl ...
In other words, the following rules won't work as expected:
JS%.cpp : %.idl
perl generate-bindings.pl ...
or
JS%.cpp : %.cpp-needs-to-be-generated
perl generate-bindings.pl ...
=====================
Is the hash idea too hacky?
supplemental_dependency.tmp : $(ALL_IDL_FILES)
perl resolve-supplemental.pl $(ALL_IDL_FILES) # Generates supplemental_dependency.tmp, with a hash of each IDL.
supplemental_dependency_with_effective_timestamp.tmp : supplemental_dependency.tmp
cp ... # copy supplemental_dependency.tmp to supplemental_dependency_with_effective_timestamp.tmp only when the bytes differ.
JS%.cpp : %.idl supplemental_dependency_with_effective_timestamp.tmp
perl generate-bindings.pl supplemental_dependency_with_effective_timestamp.tmp $< # Generates JS%.h and JS%.cpp.
(In reply to comment #16) > I guess that we need to guarantee that generate-bindings.pl for each IDL must run after all supplemental dependencies are resolved. This implies that JS%.cpp must (directly or indirectly) depend on a file that describes all the supplemental dependencies (i.e. supplemental_dependency.tmp) as a explicit rule in makefile, like this: > > JS%.cpp : %.idl supplemental_dependency.tmp > perl generate-bindings.pl ... > > In other words, the following rules won't work as expected: > > JS%.cpp : %.idl > perl generate-bindings.pl ... > > or > > JS%.cpp : %.cpp-needs-to-be-generated > perl generate-bindings.pl ... > > > ===================== > > Is the hash idea too hacky? > > supplemental_dependency.tmp : $(ALL_IDL_FILES) > perl resolve-supplemental.pl $(ALL_IDL_FILES) # Generates supplemental_dependency.tmp, with a hash of each IDL. > > supplemental_dependency_with_effective_timestamp.tmp : supplemental_dependency.tmp > cp ... # copy supplemental_dependency.tmp to supplemental_dependency_with_effective_timestamp.tmp only when the bytes differ. > > JS%.cpp : %.idl supplemental_dependency_with_effective_timestamp.tmp > perl generate-bindings.pl supplemental_dependency_with_effective_timestamp.tmp $< # Generates JS%.h and JS%.cpp. Adam, Darin: I would like to fix the issue somehow before starting WebKit modularization. If the hash idea would be acceptable, I am happy to implement it. Or I would like to look for another idea based on the ideas you suggested above. > Adam, Darin: I would like to fix the issue somehow before starting WebKit modularization. If the hash idea would be acceptable, I am happy to implement it. Or I would like to look for another idea based on the ideas you suggested above. The approach in Comment #17 will still cause all the CPP files to be regenerated whenever any IDL file changes. I thought about this for a while but I couldn't come up with anything great. I think my make foo isn't strong enough. The thing I don't know how to do is to make one thing run after another but not be dependent on it. Is there an idiom in Make for doing that? If so, Darin's approach from Comment #13 seems fine. (In reply to comment #18) > The thing I don't know how to do is to make one thing run after another but not be dependent on it. Is there an idiom in Make for doing that? The idiom is to put a phony target in the list of "all" dependencies before $(JS_DOM_HEADERS). That operation will run before the JS%.h rules runs. The order of dependencies matters. (In reply to comment #18) > > Adam, Darin: I would like to fix the issue somehow before starting WebKit modularization. If the hash idea would be acceptable, I am happy to implement it. Or I would like to look for another idea based on the ideas you suggested above. > > The approach in Comment #17 will still cause all the CPP files to be regenerated whenever any IDL file changes. Thanks Adam, now I got it. A dependency DAG of Makefile is created statically (i.e. before any rule is fired)... Specifically, B : A X C : B Y Even if X does not update B, Y is fired. > The thing I don't know how to do is to make one thing run after another but not be dependent on it. Is there an idiom in Make for doing that? If so, Darin's approach from Comment #13 seems fine. Yeah. Another problem is whether qmake, cmake and GYP also have the similar idiom. Another idea is to use "recursive make", i.e. generate the required dependency dynamically. But I am not sure if it is possible in qmake, cmake and even GYP. (In reply to comment #19) > (In reply to comment #18) > > The thing I don't know how to do is to make one thing run after another but not be dependent on it. Is there an idiom in Make for doing that? > > The idiom is to put a phony target in the list of "all" dependencies before $(JS_DOM_HEADERS). That operation will run before the JS%.h rules runs. The order of dependencies matters. Thanks Darin, maybe more modest way is "order-only-prerequisites": http://www.gnu.org/software/make/manual/make.html#Prerequisite-Types I guess this is what we need. I'll try Darin's idea using "order-only-prerequisites". But I am not still sure the similar idiom is available in qmake, cmake and GYP. (In reply to comment #19) > (In reply to comment #18) > > The thing I don't know how to do is to make one thing run after another but not be dependent on it. Is there an idiom in Make for doing that? > > The idiom is to put a phony target in the list of "all" dependencies before $(JS_DOM_HEADERS). That operation will run before the JS%.h rules runs. The order of dependencies matters. Darin: Sorry if I am misunderstanding your idea, but I am afraid that the phony target won't work well if make runs in parallel. Here are experimental results. #### test.mk from here #### all: phony A.cpp B.cpp dependency.txt: A.idl B.idl touch $@ %.touch: %.idl dependency.txt touch $@ %.cpp: %.touch touch $@ .PHONY: phony phony: A.touch B.touch sleep 5 echo "sleep end" #### test.mk to here #### #### The result from here #### $ touch A.idl && make -j 2 -f test.mk touch dependency.txt touch A.touch touch B.touch touch A.cpp sleep 5 touch B.cpp echo "sleep end" sleep end #### The result to here #### As you can see, "touch A.cpp" and "touch B.cpp" are fired without waiting for the phony. Below is the result of order-only-prerequisites. #### test2.mk from here #### all: A.cpp B.cpp dependency.txt: A.idl B.idl touch $@ A.touch: A.idl dependency.txt touch $@ B.touch: B.idl dependency.txt sleep 5 touch $@ %.cpp: %.touch | A.touch B.touch touch $@ #### test2.mk to here #### #### The result from here #### $ touch A.idl && make -j 2 -f test2.mk touch dependency.txt touch A.touch sleep 5 touch B.touch touch A.cpp touch B.cpp #### The result to here #### As you can see, "touch A.cpp" and "touch C.cpp" are fired after waiting "touch A.cpp" and "touch B.cpp". If there's work that remains here, this bug needs to be reopened, or a new one filed. If reusing this bug, please fix the title. Let me reopen this bug as a meta bug to track the issue. (In reply to comment #22) > I am afraid that the phony target won't work well if make runs in parallel. Good point. I think we have solved this in the past with multiple makefiles. I’ll let you know if I think of anything more elegant. (In reply to comment #25) > Good point. I think we have solved this in the past with multiple makefiles. I’ll let you know if I think of anything more elegant. Thanks! For now I am trying to make a patch using order-only-prerequisites in DerivedSources.make for AppleWebKit. Darin, Adam: I implemented Darin's idea in AppleWebKit, but it did not work...
supplemental_dependency.tmp : $(ALL_IDLS)
perl resolve-supplemental.pl $(ALL_IDLS) # Generates supplemental_dependency.tmp
%.idl-needs-rebuild : %.idl supplemental_dependency.tmp
perl update-idl-needs-rebuild.pl $< $@ # Updates %.idl-needs-rebuild only if the dependency about %.idl is changed
JS%.cpp : %.idl-needs-rebuild | *.idl-needs-rebuild
perl generate-bindings.pl supplemental_dependency.tmp $< # Generates JS%.cpp
Please consider the case where we edit Element.idl.
(1) resolve-supplemental.pl generates supplemental_dependency.tmp
(2) update-idl-needs-rebuild.pl runs for all IDLs, especially for Element.idl, but it does not update Element.idl-needs-rebuild because the dependency about Element.idl is not changed
(3) Consequently, generate-bindings.pl does not run for Element.idl...
Maybe we can solve this issue by adding a "hash" of Element.idl to Element.idl-needs-rebuild, so that update-idl-needs-rebuild.pl can touch Element.idl-needs-rebuild when the dependency about Element.idl is changed *and* Element.idl itself is changed. However, if we use a hash, my previous hash idea would be simpler.
What I've been misunderstanding is the following point:
> A dependency DAG of Makefile is created statically (i.e. before any rule is fired)... Specifically,
>
> B : A
> X
>
> C : B
> Y
>
> Even if X does not update B, Y is fired.
This is wrong. If X does not update B, then Y does not fire. But if so, I think that my previous hash idea might work well.
Any idea?
Adam, Darin: Sorry for the looooong discussion.
I discussed with Adam Barth in IRC and reached the following trick:
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 file'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
But... I found that it might not work in parallel. The problem is that update-idl-needs-rebuild.pl can touch xxxx.idl-needs-rebuild that does not appear on the left hand side of the make rule, and in that case "| *.idl-needs-rebuild" does not guarantee that the update of xxxx.idl-needs-rebuild has finished.
For simplicity, consider the following example:
=========================
all : A B
X : foo
sleep 3
touch Y # Touches a file that does not appear on the left hand side
Y : foo
A : X
touch A
B : Y | X
touch B
=========================
The result is as follows:
$ touch foo
$ make -j 2 # First try
sleep 3
touch Y
touch A
$ make -j 2 # Second try
sleep 3
touch Y
touch A
touch B
$ make -j 2 # Third try
sleep 3
touch Y
touch A
$ make -j 2 # Fourth try
sleep 3
touch Y
touch A
touch B
Thus, "touch B" is certainly executed after "touch Y", but "touch B" is not always executed. That's a problem...
Maybe can we use "recursive make"? (But in that case it will become more difficult to implement the same logic in GYP)
Please ignore Comment #28, that observation was wrong. I think this problem is gone. |