Bug 160739 - EWS should check if the patch is still valid before executing every major step
Summary: EWS should check if the patch is still valid before executing every major step
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-10 10:25 PDT by Aakash Jain
Modified: 2024-03-12 11:50 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (6.76 KB, patch)
2016-08-10 10:37 PDT, Aakash Jain
ap: review+
Details | Formatted Diff | Diff
Updated patch (6.59 KB, patch)
2016-08-10 13:54 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2016-08-10 10:25:49 PDT
Most of the time, it's wasteful for EWS bots to keep building or testing once a patch has become invalid (e.g.: obsoleted or r-).
It would be much better to drop obsoleted patches quickly, and to move to ones that people are actually waiting for.

One step in this direction is for EWS to check if patch is still valid before executing every major step.

Also see <rdar://problem/27768813>
Comment 1 Aakash Jain 2016-08-10 10:37:28 PDT
Created attachment 285733 [details]
Proposed patch

This does not stop the processing once the build/command has started, but checks for validation more frequently. For e.g.: if the patch fails to build, before attempting _build_without_patch, we would validate and stop.

Note: This fails unit tests because of the change in logic, I will fix it. I want to get feedback on the logic first.
Comment 2 Alexey Proskuryakov 2016-08-10 10:40:50 PDT
Comment on attachment 285733 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285733&action=review

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:88
> +        self.validate()

Do you have a list of steps that now validate, and they didn't before? It's not obvious to me if this change provides a noticeable improvement - the real benefit would come from the ability to interrupt builds and tests, which would be a much larger change.
Comment 3 Aakash Jain 2016-08-10 11:14:29 PDT
This patch perform validation before any step which executes _run_command(). There are 8 such methods: _build_without_patch, _build_and_test_without_patch*(), _clean(), _update(), _apply(), _build(), _land() and _test().

Earlier we were doing validate before clean() and if validation pass, we run all the next steps. I think the major improvement in this patch would be to skip:  _build_without_patch, _build_and_test_without_patch and _test. 

For e.g.: _build_without_patch might take significant time, we can skip it for obsolete patches.
Comment 4 Alexey Proskuryakov 2016-08-10 11:22:04 PDT
Comment on attachment 285733 [details]
Proposed patch

Makes sense, that's a non-trivial improvement.

There is some risk of reporting tools being broken by not running all the steps, we should look out for that.
Comment 5 Aakash Jain 2016-08-10 13:54:56 PDT
Created attachment 285756 [details]
Updated patch

Simplified the patch and updated unit-tests. Please review again.
Comment 6 WebKit Commit Bot 2016-08-11 12:46:43 PDT
Comment on attachment 285756 [details]
Updated patch

Clearing flags on attachment: 285756

Committed r204382: <http://trac.webkit.org/changeset/204382>
Comment 7 WebKit Commit Bot 2016-08-11 12:46:47 PDT
All reviewed patches have been landed.  Closing bug.