Bug 49407

Summary: Windows builders kick off builds for lots irrelevant changes (e.g., rebaselining Chromium test results)
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: Tools / TestsAssignee: Adam Roben (:aroben) <aroben>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ddkilzer, dpranke, eric, mrowe, wsiegrist
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
Patch
none
Moved the code into common.config eric: review+

Adam Roben (:aroben)
Reported 2010-11-11 14:30:22 PST
Windows builders kick off builds for lots irrelevant changes (e.g., rebaselining Chromium test results)
Attachments
Patch (13.34 KB, patch)
2010-11-11 14:31 PST, Adam Roben (:aroben)
no flags
Moved the code into common.config (15.61 KB, patch)
2010-11-12 08:11 PST, Adam Roben (:aroben)
eric: review+
Adam Roben (:aroben)
Comment 1 2010-11-11 14:31:06 PST
Adam Roben (:aroben)
Comment 2 2010-11-11 14:32:43 PST
Comment on attachment 73664 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73664&action=review > WebKitTools/BuildSlaveSupport/build.webkit.org-config/config.json:214 > - "schedulers": [ { "type": "AnyBranchScheduler", "name": "trunk", "change_filter": "trunk_filter", "treeStableTimer": 45.0, > + "schedulers": [ { "type": "AnyBranchScheduler", "name": "trunk", "change_filter": "trunk_filter", "treeStableTimer": 5.0, Whoops, that change was just for testing. I've reverted it locally. > WebKitTools/BuildSlaveSupport/build.webkit.org-config/config.json:224 > + { "type": "PlatformSpecificScheduler", "platform": "win", "branch": "trunk", "treeStableTimer": 5.0, And that should be "treeStableTimer": 45.0, too.
Mark Rowe (bdash)
Comment 3 2010-11-11 14:38:59 PST
Comment on attachment 73664 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73664&action=review > WebKitTools/Scripts/webkitpy/common/net/buildbot.py:523 > + (r"(?:^|/)Makefile$", ["mac"]), These files aren’t used on the build bots so I’m not sure that it’s useful to list them here.
Adam Roben (:aroben)
Comment 4 2010-11-11 14:42:53 PST
Comment on attachment 73664 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73664&action=review >> WebKitTools/Scripts/webkitpy/common/net/buildbot.py:523 >> + (r"(?:^|/)Makefile$", ["mac"]), > > These files aren’t used on the build bots so I’m not sure that it’s useful to list them here. I'll change ["mac"] to [] so they won't trigger any builds.
Eric Seidel (no email)
Comment 5 2010-11-11 15:17:27 PST
Comment on attachment 73664 [details] Patch I just moved buildbot.py sadly. :( So I broke this patch. But git shoudl be smart enough to do the right thing.
Eric Seidel (no email)
Comment 6 2010-11-11 15:18:54 PST
Comment on attachment 73664 [details] Patch I guess I wouldn't put this in buildbot. Maybe common.config? I'm just not sure what the new class should be called.
Adam Roben (:aroben)
Comment 7 2010-11-11 15:30:46 PST
(In reply to comment #5) > (From update of attachment 73664 [details]) > I just moved buildbot.py sadly. :( So I broke this patch. But git shoudl be smart enough to do the right thing. Unless I'm mistaken, you moved bugzilla.py, not buildbot.py. (In reply to comment #6) > (From update of attachment 73664 [details]) > I guess I wouldn't put this in buildbot. Maybe common.config? I'm just not sure what the new class should be called. I don't really understand the concept of common.config. What is it a config for?
Eric Seidel (no email)
Comment 8 2010-11-11 15:38:27 PST
(In reply to comment #7) > (In reply to comment #5) > > (From update of attachment 73664 [details] [details]) > > I just moved buildbot.py sadly. :( So I broke this patch. But git shoudl be smart enough to do the right thing. > > Unless I'm mistaken, you moved bugzilla.py, not buildbot.py. Oh, you're right. :) > (In reply to comment #6) > > (From update of attachment 73664 [details] [details]) > > I guess I wouldn't put this in buildbot. Maybe common.config? I'm just not sure what the new class should be called. > > I don't really understand the concept of common.config. What is it a config for? The idea originally was to have a config module/python file to hold all the webkit specific goop. the general concept of a python module which knows how to talk to bugzilla or svn or git or buildbot isn't specific to webkit. In this case you're adding information into webkitpy specific to our current file-path layout. Seems like a config file to me, whether encoded as .py file or not. Not a big deal either way, but I'm not sure it has much to do with the "how to talk to buildbot" client library that buildbot.py seems to be.
Adam Roben (:aroben)
Comment 9 2010-11-12 08:11:52 PST
Created attachment 73747 [details] Moved the code into common.config
Eric Seidel (no email)
Comment 10 2010-11-12 17:58:42 PST
Comment on attachment 73747 [details] Moved the code into common.config View in context: https://bugs.webkit.org/attachment.cgi?id=73747&action=review > WebKitTools/Scripts/webkitpy/common/config/build.py:68 > + ("chromium", ["chromium"]), Does this imply all chromium bots? > WebKitTools/Scripts/webkitpy/common/config/build.py:72 > + ("gpu", ["chromium"]), Really? Only chromium? > WebKitTools/Scripts/webkitpy/common/config/build.py:80 > + ("skia", ["chromium"]), I'm not sure skia is just chromium these days.
Adam Roben (:aroben)
Comment 11 2010-11-15 14:03:24 PST
(In reply to comment #10) > (From update of attachment 73747 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73747&action=review > > > WebKitTools/Scripts/webkitpy/common/config/build.py:68 > > + ("chromium", ["chromium"]), > > Does this imply all chromium bots? > > > WebKitTools/Scripts/webkitpy/common/config/build.py:72 > > + ("gpu", ["chromium"]), > > Really? Only chromium? > > > WebKitTools/Scripts/webkitpy/common/config/build.py:80 > > + ("skia", ["chromium"]), > > I'm not sure skia is just chromium these days. Ok, I can look into whether these are accurate. As stated in the ChangeLog and in FIXMEs, the mappings are only known to be correct at the level of Windows vs. not-Windows. There are probably other mistakes, too. My intention was that they would be discovered, fixed, and tested if/when someone tries to get this code working for some other platform. Eric, do you have any comments on the general approach here?
Eric Seidel (no email)
Comment 12 2010-11-15 14:57:10 PST
Comment on attachment 73747 [details] Moved the code into common.config I think the general approach is scarily brittle. Part of me wonders why we don't just parse the project files for this sort of information. And then I wonder why the build systems don't just have faster null builds. I'm not sure I fully understand the issue for the windows bot. Is the null build on windows really slow?
Eric Seidel (no email)
Comment 13 2010-11-15 14:57:54 PST
I guess the result of a false-positive is only a skipped build on the bots. Which isn't that bad.
Mark Rowe (bdash)
Comment 14 2010-11-15 15:28:34 PST
(In reply to comment #12) > (From update of attachment 73747 [details]) > I think the general approach is scarily brittle. > > Part of me wonders why we don't just parse the project files for this sort of information. And then I wonder why the build systems don't just have faster null builds. > > I'm not sure I fully understand the issue for the windows bot. Is the null build on windows really slow? There are two issues here: 1) Running the tests takes a constant amount of time, no matter how big the code change. 2) Builds with no changes have a non-trivial amount of overhead. 1 is always going to be a big issue. Improving 2 would likely help even in the case of builds where there are changes present since it looks to be a surprisingly big amount of overhead: * The “update from SVN” portion of the build takes around a minute for even small changes on the Mac OS X slaves. It’s more like two or three minutes on the Windows build slaves. * The build itself takes around 90 seconds on Mac and 60 seconds on Windows. * Uploading the built product to make it accessible to the slaves takes around 4 minutes on Mac and 2 minutes on Windows. All told it’s at least six minutes for a no-op build.
Eric Seidel (no email)
Comment 15 2010-11-15 15:47:37 PST
(In reply to comment #14) > (In reply to comment #12) > > (From update of attachment 73747 [details] [details]) > > I think the general approach is scarily brittle. > > > > Part of me wonders why we don't just parse the project files for this sort of information. And then I wonder why the build systems don't just have faster null builds. > > > > I'm not sure I fully understand the issue for the windows bot. Is the null build on windows really slow? > > There are two issues here: > 1) Running the tests takes a constant amount of time, no matter how big the code change. > 2) Builds with no changes have a non-trivial amount of overhead. > > 1 is always going to be a big issue. Improving 2 would likely help even in the case of builds where there are changes present since it looks to be a surprisingly big amount of overhead: Yes. I clearly wasn't considering testing time. > * The “update from SVN” portion of the build takes around a minute for even small changes on the Mac OS X slaves. It’s more like two or three minutes on the Windows build slaves. Seems we have to pay that cost at some point regardless. > * The build itself takes around 90 seconds on Mac and 60 seconds on Windows. > * Uploading the built product to make it accessible to the slaves takes around 4 minutes on Mac and 2 minutes on Windows. > > All told it’s at least six minutes for a no-op build. I think the architecture you created is fine. I'm just concerned this whole approach is a brittle bandaid. I'm certainly not objecting to this landing. Thank you Mark for further background. Seems we have more optimization we could perform for all builds.
Eric Seidel (no email)
Comment 16 2010-11-15 15:48:03 PST
Comment on attachment 73747 [details] Moved the code into common.config Without better ideas as to how to do this, r=me.
Mark Rowe (bdash)
Comment 17 2010-11-15 15:50:25 PST
(In reply to comment #15) > (In reply to comment #14) > > > * The “update from SVN” portion of the build takes around a minute for even small changes on the Mac OS X slaves. It’s more like two or three minutes on the Windows build slaves. > > Seems we have to pay that cost at some point regardless. On Windows at least a significant part of that time is spent running “svnversion”. I’m not convinced that this is necessary, but buildbot insists on it being run. It may be possible to improve the Windows times by eliminating this part of the work.
William Siegrist
Comment 18 2010-11-16 08:53:42 PST
Patch seems fine to me. As for the use of svnversion, buildbot already has a bug <http://buildbot.net/trac/ticket/167> which suggests replacing svnversion with "svn info".
Adam Roben (:aroben)
Comment 19 2010-11-16 09:42:17 PST
William Siegrist
Comment 20 2010-11-16 09:48:52 PST
Master restart started.
Note You need to log in before you can comment on or make changes to this bug.