Bug 49407 - Windows builders kick off builds for lots irrelevant changes (e.g., rebaselining Chromium test results)
Summary: Windows builders kick off builds for lots irrelevant changes (e.g., rebaselin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Adam Roben (:aroben)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-11 14:30 PST by Adam Roben (:aroben)
Modified: 2010-11-16 09:48 PST (History)
6 users (show)

See Also:


Attachments
Patch (13.34 KB, patch)
2010-11-11 14:31 PST, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff
Moved the code into common.config (15.61 KB, patch)
2010-11-12 08:11 PST, Adam Roben (:aroben)
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-11-11 14:30:22 PST
Windows builders kick off builds for lots irrelevant changes (e.g., rebaselining Chromium test results)
Comment 1 Adam Roben (:aroben) 2010-11-11 14:31:06 PST
Created attachment 73664 [details]
Patch
Comment 2 Adam Roben (:aroben) 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.
Comment 3 Mark Rowe (bdash) 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.
Comment 4 Adam Roben (:aroben) 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.
Comment 5 Eric Seidel (no email) 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.
Comment 6 Eric Seidel (no email) 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.
Comment 7 Adam Roben (:aroben) 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?
Comment 8 Eric Seidel (no email) 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.
Comment 9 Adam Roben (:aroben) 2010-11-12 08:11:52 PST
Created attachment 73747 [details]
Moved the code into common.config
Comment 10 Eric Seidel (no email) 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.
Comment 11 Adam Roben (:aroben) 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?
Comment 12 Eric Seidel (no email) 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?
Comment 13 Eric Seidel (no email) 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.
Comment 14 Mark Rowe (bdash) 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.
Comment 15 Eric Seidel (no email) 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.
Comment 16 Eric Seidel (no email) 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.
Comment 17 Mark Rowe (bdash) 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.
Comment 18 William Siegrist 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".
Comment 19 Adam Roben (:aroben) 2010-11-16 09:42:17 PST
Committed r72111: <http://trac.webkit.org/changeset/72111>
Comment 20 William Siegrist 2010-11-16 09:48:52 PST
Master restart started.