Bug 186257 – Allow the ramp controller to run tests that take less time than the initial ramp-up phase

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jon Lee]

Allow the ramp controller to run tests that take less time than the initial ramp-up phase

[Jon Lee]

Created attachment 341889 [details]
Patch

[Said Abou-Hallawa]

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.

[Jon Lee]

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.

[Jon Lee]

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

[Jon Lee]

Created attachment 342082 [details]
Patch

[Said Abou-Hallawa]

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.

[WebKit Commit Bot]

Comment on attachment 342082 [details]
Patch

Clearing flags on attachment: 342082

Committed r232565: <https://trac.webkit.org/changeset/232565>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40875607>