bench-api: Always get a Config from CLI flags
#5541
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes an issue that I ran into just now where benchmarking one `*.so` with `--engine-flags` was giving wildly unexpected results comparing to something without `--engine-flags`. The root cause here appears to that when specifying `--engine-flags` the CLI parsing code is used to create a `Config` and when omitted a `Config::new` instance is created. The main difference between these is that for the CLI caching is enabled by default and for `Config::new` it is not. Coupled with the fact that caching doesn't really work for the `main` branch this ended up giving wild results. The fix here is to first always use the CLI parsing code to create a `Config` to ensure that a config is consistently created. Next the `--disable-cache` flag is unconditionally passed to the CLI parsing to ensure that compilation actually happens. Once applied this enables comparing an engine without flags and an engine with flags which provides consistent results.
fitzgen
approved these changes
Jan 6, 2023
crates/bench-api/src/lib.rs
Outdated
Comment on lines
243
to
251
| let flags = if self.execution_flags_ptr.is_null() { | ||
| "--disable-cache".to_string() | ||
| } else { | ||
| let execution_flags = unsafe { | ||
| std::slice::from_raw_parts(self.execution_flags_ptr, self.execution_flags_len) | ||
| }; | ||
| let execution_flags = std::str::from_utf8(execution_flags) | ||
| .context("given execution flags string is not valid UTF-8")?; | ||
| format!("--disable-cache {execution_flags}") |
There was a problem hiding this comment.
Can we just do a null pointer and zero len for flags when not provided, always parse the Config from that resulting potentially-empty flags string, and then rely on #5542 for disabling the cache?
|
Merging this into #5542 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes an issue that I ran into just now where benchmarking one
*.sowith--engine-flagswas giving wildly unexpected results comparing to something without--engine-flags. The root cause here appears to that when specifying--engine-flagsthe CLI parsing code is used to create aConfigand when omitted aConfig::newinstance is created. The main difference between these is that for the CLI caching is enabled by default and forConfig::newit is not. Coupled with the fact that caching doesn't really work for themainbranch this ended up giving wild results.The fix here is to first always use the CLI parsing code to create a
Configto ensure that a config is consistently created. Next the--disable-cacheflag is unconditionally passed to the CLI parsing to ensure that compilation actually happens.Once applied this enables comparing an engine without flags and an engine with flags which provides consistent results.