Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove batching on unittest thread, use historical data to inform batching #18578

Merged
merged 4 commits into from
Sep 20, 2017

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Sep 19, 2017

This should help guarantee that the unittest thread is not unintentionally on the critical path of completion time - in my case, this improved test duration by 8 seconds - 126 seconds down to 118 seconds. This may help with perf issues you were seeing, @andy-ms ?

@weswigham
Copy link
Member Author

weswigham commented Sep 19, 2017

@andy-ms I've also now added a feature where the data for the runtime of prior test runs is used to inform the packing of batches for future sort runs. The historical data also reduces test discovery time by 1/3rd of a second by avoiding all of the stat calls, which is somewhat nice. I have not witnessed more than a second of difference in the end times of worker threads (and no more than 2 or 3 seconds on the size-informed runs); so at this point, any remaining differences in completion time would have to be harness overhead (or additional runtime caused by the addition of more tests).

As an added bonus; new tests will be run first if perf data from a prior run is available, so you can get feedback from your changes more rapidly.

@weswigham weswigham changed the title Remove batching on unittest thread Remove batching on unittest thread, use historical data to inform batching Sep 19, 2017
@ghost
Copy link

ghost commented Sep 20, 2017

Merge conflict 👇

@weswigham
Copy link
Member Author

@andy-ms rebased and resolved.

if (tasks.length === 0) {
// TODO: This indicates a particularly suboptimal packing
console.log(`Suboptimal packing detcted: no tests remain to be stolen. Reduce packing fraction from ${packfraction} to fix.`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo:detected

@weswigham weswigham merged commit 7dec4ae into microsoft:master Sep 20, 2017
@weswigham weswigham deleted the no-batch-for-unittests branch September 20, 2017 20:22
@ghost
Copy link

ghost commented Sep 21, 2017

This doesn't seem to have changed the time for me. I measured 2min 45s, compared to 2min 48s in #18462 and 2min 24s before any of this. @weswigham Could you run a comparison between c522f37 and d1c4754 on your machine?

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants