Split std_specs in 32 bits ci #8065
Conversation
|
Still. failing, but I noticed, the ci uses |
bin/ci
Outdated
| with_build_env 'SPEC_SPLIT="1%4" make std_spec threads=1' | ||
| with_build_env 'SPEC_SPLIT="2%4" make std_spec threads=1' | ||
| with_build_env 'SPEC_SPLIT="3%4" make std_spec threads=1' | ||
| with_build_env 'make compiler_spec docs threads=1' |
There was a problem hiding this comment.
You should use SPEC_SPLIT here too. This is where it's failing.
|
How's this supposed to work? If I understand the issue correctly, the compiler runs out of memory while compiling the specs. Unless I'm missing some key concept here, changing the logic in We don't need to split spec execution but already spec compilation. |
|
@straight-shoota As far as I can tell from CI's output, it's failing when running the specs, not when compiling them. |
src/spec/dsl.cr
Outdated
| # :nodoc: | ||
| def self.matches?(description, file, line, end_line = line) | ||
| spec_pattern = @@pattern | ||
| spec_line = @@line | ||
| locations = @@locations | ||
| split_filter = @@split_filter | ||
|
|
||
| if split_filter && line % split_filter[:quotient] != split_filter[:remainder] |
There was a problem hiding this comment.
The way I thought about SPLIT_SPEC, each time you do it a counter would get incremented, and it's this counter you would check against.
By using just the line number it might happen that more specs are in lines 1 modulo 4, for example.
But if it works then it's a good start :-)
There was a problem hiding this comment.
Yup, that's another way to filter them. The distribution of the std_spec at least was good enough.
$ make std_spec
7744 examples, 0 failures, 0 errors, 8 pending
$ SPEC_SPLIT="0%4" make std_spec
1958 examples, 0 failures, 0 errors, 2 pending
$ SPEC_SPLIT="1%4" make std_spec
1856 examples, 0 failures, 0 errors, 2 pending
$ SPEC_SPLIT="2%4" make std_spec
1958 examples, 0 failures, 0 errors, 2 pending
$ SPEC_SPLIT="3%4" make std_spec
1972 examples, 0 failures, 0 errors, 2 pending
|
Maybe we can try running all specs (like this, splitted) with |
|
Hm, yeah it seems you're right. Compiling the specs is not the issue. But when it's at runtime, this might actually mean there are some memory leaks. So |
bcardiff
force-pushed
the
split-specs
branch
2 times, most recently
from
7435599 to
4a6611e
Compare
|
@straight-shoota Oh, I know for sure there's a memory leak. When we compiler specs without the prelude the Unfortunately I can't think of an easy way to fix that. |
|
@asterite I see. There is the option to disable JIT compilation and run specs without prelude in a separate process as well. So it's a trade-off between speed and memory. I'm not sure how much a difference it makes, is it worth looking into that? But anyway, this is just relevant for specs and not a critical problem. So as long as we get the 32-bit specs to work by splitting them up, I guess this should be fine. |
|
Another idea, just from the top of the head: Maybe we could somehow rig the |
|
The issue isn't just for 32bits, in general too much memory is used for tests. It would be better to fix the root cause instead. The memory in the CI isn't infinite, and the limit might be eventually reached in the future even for 64bit. It also impacts devs who are using machines with not much RAM. |
|
j8r PRs and ideas are welcome. This just affects us Crystal developers. I think this fix is fine and it let's us move forward. |
|
I'm pushing temporarily @asterite's commit to check if that fix the ci. Other than that the PR is ready for review and squash |
* Skip some specs for 32 bits * Restrict threads used in the compiler during the specs * Split std_specs in 32 bits ci
* Skip some specs for 32 bits * Restrict threads used in the compiler during the specs * Split std_specs in 32 bits ci
Only for std_specs run against the compiled compiler which seems to be the one failing
This introduces a way to run a subset of the specs via an env var
SPEC_SPLIT. Is not expected to be used as a public API but is a workaround to help a bit the CI.Let's see how it works...