Bug 186062 - CommitSet range bisector should use commits occur in commit sets which specify the range as valid commits for commits without ordering.
Summary: CommitSet range bisector should use commits occur in commit sets which specif...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-29 14:56 PDT by dewei_zhu
Modified: 2018-06-05 23:10 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.22 KB, patch)
2018-05-29 14:59 PDT, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (10.09 KB, patch)
2018-06-05 21:11 PDT, dewei_zhu
rniwa: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.78 MB, application/zip)
2018-06-05 23:10 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description dewei_zhu 2018-05-29 14:56:39 PDT
CommitSet range bisector should use commits occur in commit sets which specify the range as valid commits for commits without ordering.
Comment 1 dewei_zhu 2018-05-29 14:59:55 PDT
Created attachment 341522 [details]
Patch
Comment 2 Ryosuke Niwa 2018-05-29 15:08:39 PDT
Comment on attachment 341522 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:8
> +        For commits without ordering, we should use the commits occurs in the commit sets which specify the range as valid commits.

What do you mean by "the commits occurs in the commit sets"?
Comment 3 dewei_zhu 2018-06-01 14:36:56 PDT
Comment on attachment 341522 [details]
Patch

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

>> Websites/perf.webkit.org/ChangeLog:8
>> +        For commits without ordering, we should use the commits occurs in the commit sets which specify the range as valid commits.
> 
> What do you mean by "the commits occurs in the commit sets"?

Let's say we cannot order commits for WebKit (r100, r102) due to lack of commit time and commit order. Then we should make sure any commit set get chosen, the commit for WebKit should be either r100 or r102.
Comment 4 Ryosuke Niwa 2018-06-01 15:05:03 PDT
Comment on attachment 341522 [details]
Patch

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

>>> Websites/perf.webkit.org/ChangeLog:8
>>> +        For commits without ordering, we should use the commits occurs in the commit sets which specify the range as valid commits.
>> 
>> What do you mean by "the commits occurs in the commit sets"?
> 
> Let's say we cannot order commits for WebKit (r100, r102) due to lack of commit time and commit order. Then we should make sure any commit set get chosen, the commit for WebKit should be either r100 or r102.

We should probably order lexicologically by the revision / commit hash / version number in that case. 
If the order changes from time to time, that could spell a disaster.
Comment 5 dewei_zhu 2018-06-05 21:11:19 PDT
Created attachment 342028 [details]
Patch
Comment 6 Ryosuke Niwa 2018-06-05 21:34:03 PDT
Comment on attachment 342028 [details]
Patch

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

r=me assuming you fix the comparator & add a unit test.

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:104
> +                return firstCommit.revision() < secondCommit.revision();

This can't be write. We need to be returning 0, -1, or 1.
Please add a test case which catches this bug.
Comment 7 Build Bot 2018-06-05 23:10:37 PDT
Comment on attachment 342028 [details]
Patch

Attachment 342028 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8026194

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Comment 8 Build Bot 2018-06-05 23:10:49 PDT
Created attachment 342035 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit