-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: allow limiting system resources in compilation processes #2500
base: main-v0.13.4
Are you sure you want to change the base?
Conversation
ea57122
to
ed333fb
Compare
b51ee76
to
0b23415
Compare
Benchmark movements: |
0b23415
to
b45ffe5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2500 +/- ##
===========================================
+ Coverage 40.10% 77.12% +37.01%
===========================================
Files 26 394 +368
Lines 1895 42623 +40728
Branches 1895 42623 +40728
===========================================
+ Hits 760 32872 +32112
- Misses 1100 7455 +6355
- Partials 35 2296 +2261 ☔ View full report in Codecov by Sentry. |
b45ffe5
to
69aff3e
Compare
Benchmark movements: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 5 files at r2.
Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @liorgold2, @noaov1, and @Yoni-Starkware)
crates/starknet_sierra_compile/src/resource_limits_test.rs
line 0 at r2 (raw file):
Add a test case for a simple CI command, with all resources limited, but not blocking the command.
crates/starknet_sierra_compile/src/resource_limits_test.rs
line 22 at r2 (raw file):
#[rstest] fn test_memory_size_limit() { let memory_size_limits = ResourcesLimits::new(None, None, Some(100000));
Will the test still be substantial that way? It would take less resources.
Make sure it takes as few resources for CI.
Suggestion:
let memory_size_limits = ResourcesLimits::new(None, None, Some(100));
crates/starknet_sierra_compile/src/resource_limits_test.rs
line 38 at r2 (raw file):
file_size_limits.apply(&mut command); command.spawn().expect("Failed to start disk consuming process").wait().unwrap(); assert!(std::fs::metadata("/tmp/file.txt").unwrap().len() <= 100);
Make the test more consise, and take less resources.
Suggestion:
let file_size_limit = 10;
let file_size_limits = ResourcesLimits::new(None, Some(file_size_limit), None);
let mut command = Command::new("bash");
command.args(["-c", "echo 0 > /tmp/file.txt; while true; do echo 0 >> /tmp/file.txt; done;"]);
file_size_limits.apply(&mut command);
command.spawn().expect("Failed to start disk consuming process").wait().unwrap();
assert_eq!(std::fs::metadata("/tmp/file.txt").unwrap().len(), file_size_limit); // Or whatever the resoult should be.
crates/starknet_sierra_compile/src/resource_limits.rs
line 11 at r2 (raw file):
pub mod test; #[allow(dead_code)]
Can we set this flag on the file level?
Code quote:
#[allow(dead_code)]
crates/starknet_sierra_compile/src/resource_limits.rs
line 17 at r2 (raw file):
hard: u64, units: String, }
Partway through writing this, I partially regretted that. I am leaving the comment more as a discussion than a suggestion.
If I understand the use case correctly, the Resource
is implied.
The method set
can have two parameters. &self, resource: Resource
.
You can even define an iterator over ResourcesLimits
that returns tuples (Resource, ResourceLimits)
.
Suggestion:
struct ResourceLimits {
soft: u64,
hard: u64,
units: String,
}
crates/starknet_sierra_compile/src/resource_limits.rs
line 91 at r2 (raw file):
// 2. No heap allocations occur in the `set` method. // 3. `setrlimit` is an async-signal-safe system call, which means it is safe to invoke // after `fork`.
Did this block of text pass through Grammerly/ ChatGPT? Looks good to me.
Code quote:
// The `pre_exec` method runs a given closure after forking the parent process and
// before the exec of the child process.
//
// This closure will be run in the context of the child process after a `fork`. This
// primarily means that any modifications made to memory on behalf of this closure will
// **not** be visible to the parent process. This is often a very constrained
// environment where normal operations like `malloc`, accessing environment variables
// through [`std::env`] or acquiring a mutex are not guaranteed to work (due to other
// threads perhaps still running when the `fork` was run).
//
// The current closure is safe for the following reasons:
// 1. The [`ResourcesLimits`] struct is fully constructed and moved into the closure.
// 2. No heap allocations occur in the `set` method.
// 3. `setrlimit` is an async-signal-safe system call, which means it is safe to invoke
// after `fork`.
Previously, ArniStarkware (Arnon Hod) wrote…
I don't fully understand your suggestion. How can I limit a resource without knowing what resource it is? How would I know what units this resource is counted in? The point of this struct is to hold all the information needed for using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @liorgold2, @noaov1, and @Yoni-Starkware)
crates/starknet_sierra_compile/src/resource_limits.rs
line 17 at r2 (raw file):
Previously, avi-starkware wrote…
I don't fully understand your suggestion. How can I limit a resource without knowing what resource it is? How would I know what units this resource is counted in?
The point of this struct is to hold all the information needed for using
setrlimit()
and for displaying a short log message indicating what happened.
The point is that in the context of the struct ResourcesLimits
(Plural), it is clear from the context that cpu_time.resource = Resource::CPU
. Thus, it is redundant as a member of ResourceLimits
(Singular).
crates/starknet_sierra_compile/src/resource_limits.rs
line 0 at r2 (raw file):
The names ResoureceLimits
vs ResourcesLimits
is confusing.
Previously, ArniStarkware (Arnon Hod) wrote…
This gives the process 100 bytes of memory. The process can't start with this limitation in place. 1KB is also not enough. With 10KB it works locally for me, but fails in the CI. |
Previously, ArniStarkware (Arnon Hod) wrote…
I think the more important question is whether my explanation is easy to understand and whether it justifies using an |
Previously, avi-starkware wrote…
I agree the question you raise is more important. I asked another, less important question :) |
Previously, avi-starkware wrote…
Got it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @liorgold2, @noaov1, and @Yoni-Starkware)
Previously, ArniStarkware (Arnon Hod) wrote…
The name cpu_time is the name of the field I made up. Resource::CPU is the enum value to be passed to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @liorgold2, @noaov1, and @Yoni-Starkware)
crates/starknet_sierra_compile/src/resource_limits.rs
line 17 at r2 (raw file):
Previously, avi-starkware wrote…
The name cpu_time is the name of the field I made up. Resource::CPU is the enum value to be passed to
setrlimit()
. I can have amatch
to resolve the correct resource entry, or I can just remove the structResourceLimits
entirely if you think it is not needed.
I have a suggestion in another thread.
crates/starknet_sierra_compile/src/resource_limits.rs
line 71 at r2 (raw file):
.flatten() .try_for_each(|resource_limit| resource_limit.set()) }
What do you think?
Suggestion:
#[allow(dead_code)]
struct ResourcesLimits {
pub cpu_time: Option<u64>,
pub file_size: Option<u64>,
pub memory_size: Option<u64>,
}
impl ResourcesLimits {
fn cpu_time_resource(&self) -> Option<ResourceLimits> {
self.cpu_time.map(|cpu_time| ResourceLimits {
resource: Resource::CPU,
soft: cpu_time,
hard: cpu_time,
units: "seconds".to_string(),
})
}
fn file_size_resource(&self) -> Option<ResourceLimits> {
self.file_size.map(|file_size| ResourceLimits {
resource: Resource::FSIZE,
soft: file_size,
hard: file_size,
units: "bytes".to_string(),
})
}
fn memory_size_resource(&self) -> Option<ResourceLimits> {
self.memory_size.map(|memory_size| ResourceLimits {
resource: Resource::AS,
soft: memory_size,
hard: memory_size,
units: "bytes".to_string(),
})
}
#[allow(dead_code)]
fn set(&self) -> io::Result<()> {
[self.cpu_time_resource(), self.file_size_resource(), self.memory_size_resource()]
.iter()
.flatten()
.try_for_each(|resource_limit| resource_limit.set())
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @liorgold2, @noaov1, and @Yoni-Starkware)
crates/starknet_sierra_compile/src/resource_limits.rs
line 71 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
What do you think?
Also - the struct ResourcesLimits
and the fields should be documented.
Previously, ArniStarkware (Arnon Hod) wrote…
see: https://reviewable.io/reviews/starkware-libs/sequencer/2500#-ODl10Yx5Y3VdXseIy8z |
69aff3e
to
327a1f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @liorgold2, @noaov1, and @Yoni-Starkware)
crates/starknet_sierra_compile/src/resource_limits.rs
line 91 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
I agree the question you raise is more important. I asked another, less important question :)
I consulted chatGPT and made some changes to the text
TAL
crates/starknet_sierra_compile/src/resource_limits_test.rs
line 38 at r2 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Make the test more consise, and take less resources.
Done.
327a1f1
to
95baa22
Compare
Benchmark movements: |
43af46d
to
7b07183
Compare
Benchmark movements: full_committer_flow performance regressed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 6 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @liorgold2, @noaov1, and @Yoni-Starkware)
crates/starknet_sierra_compile/src/resource_limits_test.rs
line 23 at r7 (raw file):
Previously, avi-starkware wrote…
I couldn't find a way to check that the memory limit was indeed enforced. I just check that running an infinite loop fails at some point.
I could use an external crate called
sysinfo
that allows polling the memory usage of a running process. It only works while the process is running, so I would have to have a loop that checks the memory usage frequently until the process stops, and make sure that no sample exceeds the memory_limit. Would you like me to do that?
I added polling the memory of the child process to check it does not exceed the memory limit.
The test now takes 1.1 seconds though, because if I give the process less memory than 4.5MB, it doesn't have enough memory to start the process.
Moreover, an external crate (sysinfo) is needed to check the child-process memory usage.
PTAL
7b07183
to
dfb1250
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 6 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @liorgold2, @noaov1, and @Yoni-Starkware)
crates/starknet_sierra_compile/src/resource_limits_test.rs
line 23 at r7 (raw file):
Previously, avi-starkware wrote…
I added polling the memory of the child process to check it does not exceed the memory limit.
The test now takes 1.1 seconds though, because if I give the process less memory than 4.5MB, it doesn't have enough memory to start the process.Moreover, an external crate (sysinfo) is needed to check the child-process memory usage.
PTAL
The crate looks actively maintained:
- last commit was yesterday
- new version tag every month
Benchmark movements: |
06fea27
to
b0d955b
Compare
Benchmark movements: |
b0d955b
to
17180b8
Compare
Benchmark movements: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 5 files at r10, 1 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @avi-starkware, @liorgold2, and @Yoni-Starkware)
crates/starknet_sierra_compile/src/resource_limits_test.rs
line 23 at r7 (raw file):
I just check that running an infinite loop fails at some point.
Is there a way to check the failure reason of a spawned thread?
I think that using sysinfo may be too much.
crates/starknet_sierra_compile/src/resource_limits.rs
line 87 at r12 (raw file):
/// Apply the resource limits to a given command. This moves the [`ResourceLimits`] struct into /// a closure that is held by the given command. The closure is executed once the command is /// executed, inside the spawned child process.
Suggestion:
/// Apply the resource limits to a given command object (a representation of a child process).
/// This moves the [`ResourceLimits`] struct into a closure that is held by the given command.
/// The closure is executed right before the exec system call is invoked in the child process.
crates/starknet_sierra_compile/src/resource_limits.rs
line 100 at r12 (raw file):
// as using `malloc`, accessing environment variables through [`std::env`] or acquiring // a mutex--are not guaranteed to work, because other threads may still be running at // the time of `fork`.
Suggestion:
// a mutex--are not guaranteed to work, because after `fork`, only one thread exists in the child
// process, while there might be a few in the parent process.
17180b8
to
a72c1c1
Compare
Previously, noaov1 (Noa Oved) wrote…
I found a way to do it without using sysinfo. |
Benchmark movements: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 7 files reviewed, 1 unresolved discussion (waiting on @liorgold2, @noaov1, and @Yoni-Starkware)
crates/starknet_sierra_compile/src/resource_limits.rs
line 87 at r12 (raw file):
/// Apply the resource limits to a given command. This moves the [`ResourceLimits`] struct into /// a closure that is held by the given command. The closure is executed once the command is /// executed, inside the spawned child process.
Done . (minus the parenthesis)
A command object is not a representation of a child process. It represents a command that once executed would generate a child process.
crates/starknet_sierra_compile/src/resource_limits.rs
line 100 at r12 (raw file):
// as using `malloc`, accessing environment variables through [`std::env`] or acquiring // a mutex--are not guaranteed to work, because other threads may still be running at // the time of `fork`.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @liorgold2, and @Yoni-Starkware)
crates/starknet_sierra_compile/src/resource_limits.rs
line 100 at r12 (raw file):
Previously, avi-starkware wrote…
Done.
Is it? :)
crates/starknet_sierra_compile/src/resource_limits_test.rs
line 23 at r7 (raw file):
Previously, avi-starkware wrote…
I found a way to do it without using sysinfo.
Nice!
crates/starknet_sierra_compile/src/resource_limits_test.rs
line 47 at r13 (raw file):
println!("Child process is using {} KB of memory.", memory_usage_kb); assert!(memory_usage_kb * 1024 < memory_limit); thread::sleep(Duration::from_millis(250));
Consider verifying the error message instead of asserting that the memory usage is within the bounds. (non-blocking)
Code quote:
thread::sleep(Duration::from_millis(250));
a72c1c1
to
a6bdca6
Compare
Benchmark movements: |
a6bdca6
to
0603a84
Compare
Benchmark movements: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r14, 2 of 2 files at r15, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @liorgold2, and @Yoni-Starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @liorgold2, and @Yoni-Starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @liorgold2, and @Yoni-Starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @liorgold2, and @Yoni-Starkware)
No description provided.