Bug 183309 - Create analysis task should use build time as fallback when commit time is not available.
Summary: Create analysis task should use build time as fallback when commit time is no...
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-03-02 18:52 PST by dewei_zhu
Modified: 2018-03-05 17:35 PST (History)
3 users (show)

See Also:


Attachments
Patch (5.99 KB, patch)
2018-03-02 18:53 PST, dewei_zhu
rniwa: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.16 MB, application/zip)
2018-03-02 20:25 PST, 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-03-02 18:52:16 PST
Create analysis task should use build time as fallback when commit time is not available.
Comment 1 dewei_zhu 2018-03-02 18:53:59 PST
Created attachment 334946 [details]
Patch
Comment 2 Build Bot 2018-03-02 20:25:14 PST
Comment on attachment 334946 [details]
Patch

Attachment 334946 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6745646

New failing tests:
compositing/debug-borders-dynamic.html
Comment 3 Build Bot 2018-03-02 20:25:15 PST
Created attachment 334947 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 4 Ryosuke Niwa 2018-03-02 20:36:30 PST
Comment on attachment 334946 [details]
Patch

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

> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;

Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
Comment 5 dewei_zhu 2018-03-05 16:14:49 PST
Comment on attachment 334946 [details]
Patch

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

>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
> 
> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?

No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php
Comment 6 Ryosuke Niwa 2018-03-05 16:19:29 PST
Comment on attachment 334946 [details]
Patch

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

>>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
>>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
>> 
>> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
> 
> No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php

Okay. In that case, do
$first_result = array_get($result, 0, array());
return array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time'];
Comment 7 dewei_zhu 2018-03-05 16:41:23 PST
Comment on attachment 334946 [details]
Patch

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

>>>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
>>>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
>>> 
>>> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
>> 
>> No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php
> 
> Okay. In that case, do
> $first_result = array_get($result, 0, array());
> return array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time'];

Sounds a good idea. The 'time' key will always exist as long as there is a matching entry. Changing `array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time']` to `$first_result['time'] ? $first_result['time'] : $first_result['build_time']` would make it work.
Comment 8 Ryosuke Niwa 2018-03-05 17:06:33 PST
(In reply to dewei_zhu from comment #7)
> Comment on attachment 334946 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334946&action=review
> 
> >>>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
> >>>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
> >>> 
> >>> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
> >> 
> >> No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php
> > 
> > Okay. In that case, do
> > $first_result = array_get($result, 0, array());
> > return array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time'];
> 
> Sounds a good idea. The 'time' key will always exist as long as there is a
> matching entry. Changing `array_key_exists('time', $first_result) :
> $first_result['time'] : $first_result['build_time']` to
> `$first_result['time'] ? $first_result['time'] :
> $first_result['build_time']` would make it work.

Okay. We should be careful for the case when time is 0 and NULL.
Comment 9 dewei_zhu 2018-03-05 17:30:15 PST
Comment on attachment 334946 [details]
Patch

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

>>>>>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
>>>>>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
>>>>> 
>>>>> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
>>>> 
>>>> No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php
>>> 
>>> Okay. In that case, do
>>> $first_result = array_get($result, 0, array());
>>> return array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time'];
>> 
>> Sounds a good idea. The 'time' key will always exist as long as there is a matching entry. Changing `array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time']` to `$first_result['time'] ? $first_result['time'] : $first_result['build_time']` would make it work.
> 
> Okay. We should be careful for the case when time is 0 and NULL.

We've already have some code that assumes time is never to be 0. For example: line 29 of create-analysis-task.php `if (!$start_run_time || !$end_run_time || $start_run_time == $end_run_time)`
Comment 10 Ryosuke Niwa 2018-03-05 17:35:48 PST
(In reply to dewei_zhu from comment #9)
> Comment on attachment 334946 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334946&action=review
> 
> >>>>>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
> >>>>>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
> >>>>> 
> >>>>> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
> >>>> 
> >>>> No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php
> >>> 
> >>> Okay. In that case, do
> >>> $first_result = array_get($result, 0, array());
> >>> return array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time'];
> >> 
> >> Sounds a good idea. The 'time' key will always exist as long as there is a matching entry. Changing `array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time']` to `$first_result['time'] ? $first_result['time'] : $first_result['build_time']` would make it work.
> > 
> > Okay. We should be careful for the case when time is 0 and NULL.
> 
> We've already have some code that assumes time is never to be 0. For
> example: line 29 of create-analysis-task.php `if (!$start_run_time ||
> !$end_run_time || $start_run_time == $end_run_time)`

I think those are all potential bugs.