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.

Sign up for GitHub

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

Jump to bottom

Ability to disable cache after it's been configured on Config #5542

Merged
merged 4 commits into from
Jan 6, 2023
Merged

Ability to disable cache after it's been configured on Config #5542

merged 4 commits into from
Jan 6, 2023

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jan 6, 2023

No description provided.

@fitzgen fitzgen requested a review from alexcrichton January 6, 2023 20:16
@fitzgen fitzgen mentioned this pull request Jan 6, 2023
Closed
alexcrichton
alexcrichton reviewed Jan 6, 2023
View reviewed changes
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

I like Config::disable_cache, but for the update to wasmtime-bench-api would you be ok going with #5541 instead to avoid having two branches for possibly-different Config?

@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime labels Jan 6, 2023
@github-actions
Copy link

github-actions bot commented Jan 6, 2023

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@github-actions
Copy link

github-actions bot commented Jan 6, 2023 •
edited by fitzgen
Loading

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

  • If you added a new Config method, you wrote extensive documentation for
    it.

    Our documentation should be of the following form:

    Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

# Example

Optional example here.

  • If you added a new Config method, or modified an existing one, you
    ensured that this configuration is exercised by the fuzz targets.

    For example, if you expose a new strategy for allocating the next instance
    slot inside the pooling allocator, you should ensure that at least one of our
    fuzz targets exercises that new strategy.

    Often, all that is required of you is to ensure that there is a knob for this
    configuration option in wasmtime_fuzzing::Config (or one
    of its nested structs).

    Rarely, this may require authoring a new fuzz target to specifically test this
    configuration. See our docs on fuzzing for more details.

  • If you are enabling a configuration option by default, make sure that it
    has been fuzzed for at least two weeks before turning it on by default.

To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

@alexcrichton
bench-api: Always get a Config from CLI flags
4ebc6ec 
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.
alexcrichton
alexcrichton approved these changes Jan 6, 2023
View reviewed changes
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

@fitzgen want to confirm these changes all look good to you?

@fitzgen
Copy link
Member Author

fitzgen commented Jan 6, 2023

@fitzgen want to confirm these changes all look good to you?

LGTM, can't actually approve because its my own PR even tho you added commits.

@fitzgen fitzgen enabled auto-merge (squash) January 6, 2023 20:41
@fitzgen fitzgen merged commit a491eac into bytecodealliance:main Jan 6, 2023
@fitzgen fitzgen deleted the disable-cache branch January 9, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton approved these changes

Assignees
No one assigned
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fitzgen @alexcrichton