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

Tests writes and iteration with Loom #163

Merged
merged 3 commits into from
Nov 16, 2022
Merged

Tests writes and iteration with Loom #163

merged 3 commits into from
Nov 16, 2022

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Nov 14, 2022

No description provided.

Allows to instrument more code

Reduces the max number of interruptions in the CI to keep the run time small
@@ -79,3 +79,5 @@ jobs:
with:
command: test
args: --test loom --features=loom,instrumentation --release --verbose
env:
LOOM_MAX_PREEMPTIONS: 3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We limit here the number of context switch to keep the number of sequence of states that Loom checks to an amount compatible with a usable CI runtime.

@@ -155,6 +155,9 @@ impl Header {
}

pub struct Entry<B: AsRef<[u8]> + AsMut<[u8]>>(usize, B);
#[cfg(feature = "loom")]
pub type FullEntry = Entry<Vec<u8>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad hack because of the stack limitations introduced by Loom.

@arkpar
Copy link
Member

arkpar commented Nov 15, 2022

Is it possible to increase the stack size with ulimit on the CI runner instead of changing the code?

@Tpt
Copy link
Contributor Author

Tpt commented Nov 15, 2022

Is it possible to increase the stack size with ulimit on the CI runner instead of changing the code?

I tried to play with it and with RUST_MIN_STACK that configures the spawned threads limit. Sadly I have not manage to get the tests to pass without either crashing or consuming all my laptop memory (32GB). It's quite unfortunate.

@arkpar arkpar requested a review from cheme November 15, 2022 10:46
tests/loom.rs Outdated
});

t1.join().unwrap();
t2.join().unwrap();
t3.join().unwrap();

// Checks, that either T1 committed before T2 or reverse
Copy link
Collaborator

Choose a reason for hiding this comment

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

So IIUC here the assert_eq is not really what we want to test, comment may not be up to date. Would just put Check state consistency.

@Tpt Tpt merged commit ac7f61e into paritytech:master Nov 16, 2022
@Tpt Tpt deleted the more-loom branch November 16, 2022 16:15
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