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

feat: allow limiting system resources in compilation processes #2500

Open
wants to merge 1 commit into
base: main-v0.13.4
Choose a base branch
from

Conversation

avi-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch 2 times, most recently from b51ee76 to 0b23415 Compare December 10, 2024 11:07
Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.473 ms 34.747 ms 35.061 ms]
change: [+1.0113% +1.8693% +2.7680%] (p = 0.00 < 0.05)
Performance has regressed.
Found 21 outliers among 100 measurements (21.00%)
2 (2.00%) high mild
19 (19.00%) high severe

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch from 0b23415 to b45ffe5 Compare December 10, 2024 12:40
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 79.03226% with 13 lines in your changes missing coverage. Please review.

Project coverage is 77.12%. Comparing base (e3165c4) to head (730d4f0).
Report is 816 commits behind head on main.

Files with missing lines Patch % Lines
...tes/starknet_sierra_compile/src/resource_limits.rs 79.03% 12 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch from b45ffe5 to 69aff3e Compare December 10, 2024 13:30
Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.526 ms 34.848 ms 35.222 ms]
change: [+1.1114% +2.0840% +3.2384%] (p = 0.00 < 0.05)
Performance has regressed.
Found 22 outliers among 100 measurements (22.00%)
4 (4.00%) high mild
18 (18.00%) high severe

Copy link
Contributor

@ArniStarkware ArniStarkware left a 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`.

@avi-starkware
Copy link
Contributor Author

crates/starknet_sierra_compile/src/resource_limits.rs line 17 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

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).

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.

Copy link
Contributor

@ArniStarkware ArniStarkware left a 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.

@avi-starkware
Copy link
Contributor Author

crates/starknet_sierra_compile/src/resource_limits_test.rs line 22 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Will the test still be substantial that way? It would take less resources.
Make sure it takes as few resources for CI.

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.

@avi-starkware
Copy link
Contributor Author

crates/starknet_sierra_compile/src/resource_limits.rs line 91 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Did this block of text pass through Grammerly/ ChatGPT? Looks good to me.

I think the more important question is whether my explanation is easy to understand and whether it justifies using an unsafe block.

@ArniStarkware
Copy link
Contributor

crates/starknet_sierra_compile/src/resource_limits.rs line 91 at r2 (raw file):

Previously, avi-starkware wrote…

I think the more important question is whether my explanation is easy to understand and whether it justifies using an unsafe block.

I agree the question you raise is more important. I asked another, less important question :)

@ArniStarkware
Copy link
Contributor

crates/starknet_sierra_compile/src/resource_limits_test.rs line 22 at r2 (raw file):

Previously, avi-starkware 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.

Got it.

Copy link
Contributor

@ArniStarkware ArniStarkware left a 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)

@avi-starkware
Copy link
Contributor Author

crates/starknet_sierra_compile/src/resource_limits.rs line 17 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

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).

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 a match to resolve the correct resource entry, or I can just remove the struct ResourceLimits entirely if you think it is not needed.

Copy link
Contributor

@ArniStarkware ArniStarkware left a 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 a match to resolve the correct resource entry, or I can just remove the struct ResourceLimits 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())
    }

Copy link
Contributor

@ArniStarkware ArniStarkware left a 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.

@ArniStarkware
Copy link
Contributor

crates/starknet_sierra_compile/src/resource_limits.rs line 17 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I have a suggestion in another thread.

see: https://reviewable.io/reviews/starkware-libs/sequencer/2500#-ODl10Yx5Y3VdXseIy8z

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch from 69aff3e to 327a1f1 Compare December 10, 2024 17:50
Copy link
Contributor Author

@avi-starkware avi-starkware left a 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.

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch from 327a1f1 to 95baa22 Compare December 10, 2024 18:11
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.969 ms 35.030 ms 35.105 ms]
change: [-4.4176% -2.6452% -1.1579%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch 4 times, most recently from 43af46d to 7b07183 Compare December 23, 2024 15:54
Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.623 ms 35.146 ms 35.758 ms]
change: [+2.1146% +3.5892% +5.5137%] (p = 0.00 < 0.05)
Performance has regressed.
Found 16 outliers among 100 measurements (16.00%)
3 (3.00%) high mild
13 (13.00%) high severe

full_committer_flow performance regressed!
full_committer_flow time: [30.415 ms 30.476 ms 30.564 ms]
change: [+2.4429% +2.7925% +3.1274%] (p = 0.00 < 0.05)
Performance has regressed.
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe

Copy link
Contributor Author

@avi-starkware avi-starkware left a 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

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch from 7b07183 to dfb1250 Compare December 24, 2024 08:41
Copy link
Contributor Author

@avi-starkware avi-starkware left a 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

Copy link

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [30.723 ms 30.838 ms 31.003 ms]
change: [+3.3045% +3.7186% +4.2847%] (p = 0.00 < 0.05)
Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severe

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch 2 times, most recently from 06fea27 to b0d955b Compare December 24, 2024 08:55
Copy link

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [30.422 ms 30.487 ms 30.564 ms]
change: [+2.5999% +2.8971% +3.1943%] (p = 0.00 < 0.05)
Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch from b0d955b to 17180b8 Compare December 24, 2024 10:10
@avi-starkware avi-starkware changed the base branch from main to main-v0.13.4 December 24, 2024 10:11
Copy link

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [30.919 ms 30.978 ms 31.038 ms]
change: [+3.0549% +3.6418% +4.0743%] (p = 0.00 < 0.05)
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild

Copy link
Collaborator

@noaov1 noaov1 left a 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.

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch from 17180b8 to a72c1c1 Compare December 24, 2024 19:44
@avi-starkware
Copy link
Contributor Author

crates/starknet_sierra_compile/src/resource_limits_test.rs line 23 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

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.

I found a way to do it without using sysinfo.

Copy link

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [30.637 ms 30.700 ms 30.780 ms]
change: [+3.1837% +3.4327% +3.7261%] (p = 0.00 < 0.05)
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severe

Copy link
Contributor Author

@avi-starkware avi-starkware left a 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.

Copy link
Collaborator

@noaov1 noaov1 left a 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));

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch from a72c1c1 to a6bdca6 Compare December 25, 2024 09:46
Copy link

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [30.530 ms 30.576 ms 30.623 ms]
change: [+2.7307% +2.9413% +3.1525%] (p = 0.00 < 0.05)
Performance has regressed.

@avi-starkware avi-starkware force-pushed the avi/add-resource-limits-to-compilation branch from a6bdca6 to 0603a84 Compare December 25, 2024 13:59
Copy link

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [30.476 ms 30.534 ms 30.604 ms]
change: [+2.1456% +2.4056% +2.6875%] (p = 0.00 < 0.05)
Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
4 (4.00%) high mild
2 (2.00%) high severe

Copy link
Collaborator

@noaov1 noaov1 left a 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)

Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @liorgold2, and @Yoni-Starkware)

Copy link
Contributor

@ArniStarkware ArniStarkware left a 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)

Copy link
Contributor

@ArniStarkware ArniStarkware left a 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)

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.

4 participants