Bug 49407 – Windows builders kick off builds for lots irrelevant changes (e.g., rebaselining Chromium test results)

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Roben (:aroben)]

Windows builders kick off builds for lots irrelevant changes (e.g., rebaselining Chromium test results)

[Adam Roben (:aroben)]

Created attachment 73664 [details]
Patch

[Adam Roben (:aroben)]

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 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 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 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 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)]

(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)]

(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)]

Created attachment 73747 [details]
Moved the code into common.config

[Eric Seidel (no email)]

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)]

(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 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)]

I guess the result of a false-positive is only a skipped build on the bots.  Which isn't that bad.

[Mark Rowe (bdash)]

(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)]

(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 on attachment 73747 [details]
Moved the code into common.config

Without better ideas as to how to do this, r=me.

[Mark Rowe (bdash)]

(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]

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)]

Committed r72111: <http://trac.webkit.org/changeset/72111>

[William Siegrist]

Master restart started.