Skip to content

[WIP] Parallel tests #877

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

Closed

Conversation

JamesLMilner
Copy link

@JamesLMilner JamesLMilner commented Sep 29, 2019

This PR aims to start tackling #825. I've been diving in and out of it for a couple weeks, and I realise parallel testing for compiler was added. I had to undo some of that, in the hope for a better overall parallel testing structure, so I hope that's not an issue.

One thing is that at the moment the test scripts are simple procedurely execute node scripts without any clear interface / exports. This PR aims to address this and allow for execution using a parallel approach using cluster. This has the unfortunate side effect of logging everything in a non linear order. I think in the long run it would be good to introduce some structure around logging, potentially with some quiet or silent modes which just log failures at the end of the suites as the logging is quite noisey.

Hopefully this could pave the way adding more tests in a scalable fashion, as they can export the same interface and be added to the test runner.

Sync

npm run test

Has the following running times

real    0m52.663s
user    1m17.207s
sys     0m1.401s

Parallel

npm run test:parallel

real    0m28.401s
user    2m20.270s
sys     0m2.806s

So that means a parallel test will run in 53.99% of a synchronous test (approximately twice as fast).

@JamesLMilner JamesLMilner changed the title [WIPParallel tests [WIP] Parallel tests Sep 29, 2019
const path = require('path');

const tests = [
["./packages/packages/as", "node ../../../../bin/asc as/index.ts --noEmit --runtime stub --validate"],
Copy link
Author

Choose a reason for hiding this comment

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

It would be nice to inline these into the code rather than calling spawn I think.

@MaxGraey
Copy link
Member

MaxGraey commented Sep 29, 2019

@JamesMilnerUK Thanks for your effort. But most of part you PR already landed. Or you suggest some improvements?

@JamesLMilner
Copy link
Author

JamesLMilner commented Sep 29, 2019

@MaxGraey yes, I did see that, shame that both me and @dcodeIO were working on it at the same time. I should have posted some WIP earlier. The main difference for this approach is that it runs the whole set of test suites in parallel, not just the compiler.

I timed the difference and although the approach in this PR is marginally faster overall (~4s on my machine), it's probably not worth the added non-linearity it brings to the compiler tests. I'll close it out unless there's any further interest.

@MaxGraey
Copy link
Member

I think you could try improve existing parallel solution by @dcodeIO. Just add async operations. For example WebAssembly.instantiate instead synced WebAssembly.Module / WebAssembly.Instance. And convert rest synced operations to async. Currently CPUs on my machine load only on 60-70% so I guess we still have room for some improvements. Of course if you interested in here)

@dcodeIO
Copy link
Member

dcodeIO commented Sep 29, 2019

shame that both me and @dcodeIO were working on it at the same time

Sorry for that, got curious when you mentioned that the compiler test suite was just about 30% faster, so I gave that a shot to see for myself and ended up with the PR, hoping that it will be useful as a baseline for future improvements.

@JamesLMilner
Copy link
Author

@dcodeIO no worries, totally understand. I think your way was a lot less intrusive in the end anyhow!

@MaxGraey I wonder if WebAssembly.instantiate would be significantly faster? Because the work is already happening in a worker (and is therefore nonblocking async), I'm not sure moving to a Promise based approach would improve the running time.

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

Successfully merging this pull request may close these issues.

3 participants