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

[ base,tests ] Reduce dependency on contrib #1595

Merged
merged 5 commits into from
Feb 2, 2022

Conversation

CodingCellist
Copy link
Collaborator

In an attempt to minimise the number of dependencies on contrib in the core of the language, tests can be run in parallel using the lower-level fork and Channel. On the one lucky run where everything worked, this also seems to be faster, possibly due to re-using threads. However this implementation led me to discover #1552 and as such, this should not be merged until #1552 is fixed.

The previous test-runner implementation relied on `Future` from
`contrib` and spawned a new future for each test instead of re-using the
threads (expected behaviour in a pool). This commit removes the
dependency on `Future` (since `contrib` libs are generally considered
experimental) in favour of using the built-in concurrency primitives. It
also re-uses threads for running the tests in each TestPool, by sending
tests and receiving results over `Channel`s. No behaviour in the tests
is altered; they are still observed to work correctly and run in
parallel.
@CodingCellist
Copy link
Collaborator Author

Just a quick note (since there's a PR review spree going on) that this is still being worked on. Currently ironing out merging with the latest master : )

@gallais
Copy link
Member

gallais commented Jan 26, 2022

@CodingCellist This seems to be working on my machine, should we consider this ready?

@gallais gallais marked this pull request as ready for review February 1, 2022 10:12
@gallais
Copy link
Member

gallais commented Feb 1, 2022

I'm planning to merge this by the end of the week. Make yourself known if you don't think it's a good idea. :)

@CodingCellist
Copy link
Collaborator Author

@gallais Hi! Yes! Sorry! I meant to reply to this a week ago and then got distracted ^^;;

It seems to work on my machine as well, which is really neat, so I'm happy for this to be merged! I'm somewhat confused as to why it's fixed itself, but I suspect it could've been something to do with #1596...
From a bit of benchmarking on my machine, using the chez backend with 8 threads and /usr/bin/time to measure things, these changes still seems to make things slightly faster (~7 seconds faster for make test) and more resource efficient (I get ~480% CPU usage with Future vs a consistent 600% CPU usage using this branch), so I think we can just merge it : )

@gallais gallais merged commit f4b7815 into idris-lang:main Feb 2, 2022
@gallais
Copy link
Member

gallais commented Feb 2, 2022

🚢

@benhormann
Copy link
Contributor

@CodingCellist test still depends on contrib for Control.ANSI.

@gallais The change to test.ipkg broke my bootstrap script in #1992. Shall I add -p contrib or depends = contrib?

@gallais
Copy link
Member

gallais commented Feb 2, 2022

I see... We can't rely on the builds to check that these dependencies are correct because we're passing

IDRIS2_PATH="/home/runner/work/Idris2/Idris2/libs/prelude/build/ttc:/home/runner/work/Idris2/Idris2/libs/base/build/ttc:/home/runner/work/Idris2/Idris2/libs/contrib/build/ttc:/home/runner/work/Idris2/Idris2/libs/network/build/ttc:/home/runner/work/Idris2/Idris2/libs/test/build/ttc:/home/runner/work/Idris2/Idris2/libs/linear/build/ttc"

😫

I'd say put back the depends = contrib because that is the correct thing to do.

@benhormann
Copy link
Contributor

I see... We can't rely on the builds to check that these dependencies are correct because we're passing

My bootstrap script does away with that nonsense, so you'll be able to rest easy 😉

I'd say put back the depends = contrib because that is the correct thing to do.

Done.

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

Successfully merging this pull request may close these issues.

3 participants