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

Use fixed number of threads and sequential mode for reftests #2766

Closed

Conversation

@jviereck
Copy link
Contributor

jviereck commented Jul 4, 2014

This fixes #1975 and #2577.

The reference tests use now 4 threads for rendering and layout. The number is quite random. In terms of performance it would be better to choose the thread number based on the given hardware, but on the other hand a fixed number might make the tests more reproduceable. I went with the later.

Also, this PR addes new check-ref-cpu-sequential and check-ref-gpu-sequential make/test targets, which will run the tests with only one layout and render thread.

…testing as well (fixes #1975)
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jul 4, 2014

Critic review: https://critic.hoppipolla.co.uk/r/1975

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@highfive
Copy link

highfive commented Jul 4, 2014

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.

@jdm
Copy link
Member

jdm commented Jul 7, 2014

compile: reftest
/bin/sh: -c: line 0: syntax error near unexpected token `('
/bin/sh: -c: line 0: `echo check: reftests with CPU rendering (using multiple threads)'
@pcwalton
Copy link
Contributor

pcwalton commented Jul 7, 2014

Looks good to me, but I'll kick this review over to @metajack and/or @larsbergstrom because it's makefile changes.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 8, 2014

I'm sort of concerned that we would now be asking people to run all of the ref tests 4 times. Maybe we should:

  1. check-ref should run the parallel variants
  2. The scripts called from .travis.yml should be changed to either the sequential variants or both the sequential and parallel ones.

@metajack do you have an opinion?

@jdm
Copy link
Member

jdm commented Dec 11, 2014

@metajack Can we make a decision here? The age of this PR is embarrassing.

@metajack
Copy link
Contributor

metajack commented Dec 12, 2014

Since we don't use makefiles anymore, this will have to be redone as mach changes and saltfs changes for buildbot.

I think the thing to do would be to add -j N parameters to the relevant test commands in mach, and then we can call those with specific values from the builtbot logic.

I'm not sure we have to run reftests four times, but if we want coverage of most cases, we will have to run them with and without parallelism. So the buildbot change would involve a new test-ref build step that passed -j 1, in addition to changing the current build step to use -j 4. Reftests aren't a huge deal right now time wise, so this shouldn't increase the cycle time much.

@jdm jdm removed the S-needs-decision label Dec 12, 2014
@jdm
Copy link
Member

jdm commented Dec 12, 2014

Ok, let's close this PR and move that decision into the associated issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.