Bug 186257 - Allow the ramp controller to run tests that take less time than the initial ramp-up phase
Summary: Allow the ramp controller to run tests that take less time than the initial r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on: 186256
Blocks: 186259
  Show dependency treegraph
 
Reported: 2018-06-03 21:48 PDT by Jon Lee
Modified: 2018-06-06 17:48 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.82 KB, patch)
2018-06-03 21:50 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (4.04 KB, patch)
2018-06-06 14:47 PDT, Jon Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2018-06-03 21:48:41 PDT
Allow the ramp controller to run tests that take less time than the initial ramp-up phase
Comment 1 Jon Lee 2018-06-03 21:50:22 PDT
Created attachment 341889 [details]
Patch
Comment 2 Said Abou-Hallawa 2018-06-04 13:57:41 PDT
Comment on attachment 341889 [details]
Patch

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

> PerformanceTests/ChangeLog:14
> +                In general testing with really short durations isn't practical, but sometimes it is

Wrong indentation.

> PerformanceTests/MotionMark/tests/resources/main.js:382
> +                this._endTimestamp = timestamp + this._testLength;

The logic of initializing and changing this._endTimestamp and this._startTimestamp is now hard to track. We initialize them like this:

        this._startTimestamp = 0;
        this._endTimestamp = options["test-interval"];

And I would expect to see them changing at the same time like that 
 
        this._startTimestamp = timestamp;
        this._endTimestamp = timestamp + this._testLength;

But I do not see that happing always in the current code and the new changes. I see couple of places where we change this._endTimestamp only and one place where we change this._startTimestamp only. I am just worried that they may get out of sync and we end up having this._endTimestamp - this._startTimestamp != this._testLength.

I would suggest either getting rid of one of them, probably this._endTimestamp and replace all the instances of it by this._startTimestamp + this._testLength.  Or you may add a method to shift the start and end time stamps of the test. And use this method instead of directly changing only one of them.
Comment 3 Jon Lee 2018-06-06 13:57:52 PDT
Comment on attachment 341889 [details]
Patch

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

>> PerformanceTests/ChangeLog:14
>> +                In general testing with really short durations isn't practical, but sometimes it is
> 
> Wrong indentation.

It's a new paragraph. I can do it block style instead.

>> PerformanceTests/MotionMark/tests/resources/main.js:382
>> +                this._endTimestamp = timestamp + this._testLength;
> 
> The logic of initializing and changing this._endTimestamp and this._startTimestamp is now hard to track. We initialize them like this:
> 
>         this._startTimestamp = 0;
>         this._endTimestamp = options["test-interval"];
> 
> And I would expect to see them changing at the same time like that 
>  
>         this._startTimestamp = timestamp;
>         this._endTimestamp = timestamp + this._testLength;
> 
> But I do not see that happing always in the current code and the new changes. I see couple of places where we change this._endTimestamp only and one place where we change this._startTimestamp only. I am just worried that they may get out of sync and we end up having this._endTimestamp - this._startTimestamp != this._testLength.
> 
> I would suggest either getting rid of one of them, probably this._endTimestamp and replace all the instances of it by this._startTimestamp + this._testLength.  Or you may add a method to shift the start and end time stamps of the test. And use this method instead of directly changing only one of them.

Ok. I got rid of the one place where only this._startTimestamp is updated. The call to Controller.start() updates both together anyway.

It is incorrect to always update this._endTimestamp to be this._startTimestamp + testLength. In the case of the ramp controller, the first phase can take a variable length of time, depending on what order of magnitude number of particles can be rendered. We want testLength to cover the time period after we do the initial tier estimate.

Finally, I don't think we should worry about the possibility that this._startTimestamp ends up after this._endTimestamp. The timestamp monotonically increases, and the updated calculations are always based on the most recent timestamp. Except for when _startTimestamp is updated from 0 to the wall clock time in Controller.start(), it is never updated.

_startTimestamp starts out at 0, but doesn't mean anything, because it supposed to represent wall clock time. After letting the test initialize for 100 ms, we then set it to the current time. That _startTimestamp never changes.

For the controllers that aren't the ramp controller, the _endTimestamp should be _startTimestamp + testLength.

For the ramp controller, however, we have to continue to extend the length because we don't know how long phase A runs.
Comment 4 Jon Lee 2018-06-06 14:41:05 PDT
(In reply to Jon Lee from comment #3)
> Comment on attachment 341889 [details]
> 
> Ok. I got rid of the one place where only this._startTimestamp is updated.
> The call to Controller.start() updates both together anyway.
> 

Never mind. I can't get rid of the place where this._startTimestamp is updated. That is a _startTimestamp variable on the benchmark, whereas the other _startTimestamp update is in the controller.

I'll disambiguate by name.
Comment 5 Jon Lee 2018-06-06 14:47:06 PDT
Created attachment 342082 [details]
Patch
Comment 6 Said Abou-Hallawa 2018-06-06 17:39:12 PDT
Comment on attachment 341889 [details]
Patch

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

>>> PerformanceTests/MotionMark/tests/resources/main.js:382
>>> +                this._endTimestamp = timestamp + this._testLength;
>> 
>> The logic of initializing and changing this._endTimestamp and this._startTimestamp is now hard to track. We initialize them like this:
>> 
>>         this._startTimestamp = 0;
>>         this._endTimestamp = options["test-interval"];
>> 
>> And I would expect to see them changing at the same time like that 
>>  
>>         this._startTimestamp = timestamp;
>>         this._endTimestamp = timestamp + this._testLength;
>> 
>> But I do not see that happing always in the current code and the new changes. I see couple of places where we change this._endTimestamp only and one place where we change this._startTimestamp only. I am just worried that they may get out of sync and we end up having this._endTimestamp - this._startTimestamp != this._testLength.
>> 
>> I would suggest either getting rid of one of them, probably this._endTimestamp and replace all the instances of it by this._startTimestamp + this._testLength.  Or you may add a method to shift the start and end time stamps of the test. And use this method instead of directly changing only one of them.
> 
> Ok. I got rid of the one place where only this._startTimestamp is updated. The call to Controller.start() updates both together anyway.
> 
> It is incorrect to always update this._endTimestamp to be this._startTimestamp + testLength. In the case of the ramp controller, the first phase can take a variable length of time, depending on what order of magnitude number of particles can be rendered. We want testLength to cover the time period after we do the initial tier estimate.
> 
> Finally, I don't think we should worry about the possibility that this._startTimestamp ends up after this._endTimestamp. The timestamp monotonically increases, and the updated calculations are always based on the most recent timestamp. Except for when _startTimestamp is updated from 0 to the wall clock time in Controller.start(), it is never updated.
> 
> _startTimestamp starts out at 0, but doesn't mean anything, because it supposed to represent wall clock time. After letting the test initialize for 100 ms, we then set it to the current time. That _startTimestamp never changes.
> 
> For the controllers that aren't the ramp controller, the _endTimestamp should be _startTimestamp + testLength.
> 
> For the ramp controller, however, we have to continue to extend the length because we don't know how long phase A runs.

Thanks for the explanation. I was confused about the test duration and I was thinking it has to be fixed and equal to testLength always.
Comment 7 WebKit Commit Bot 2018-06-06 17:45:41 PDT
Comment on attachment 342082 [details]
Patch

Clearing flags on attachment: 342082

Committed r232565: <https://trac.webkit.org/changeset/232565>
Comment 8 WebKit Commit Bot 2018-06-06 17:45:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-06-06 17:48:05 PDT
<rdar://problem/40875607>