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

Added Benchmarking; Added Sampler to Prelude for Benchmarking #1071

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Riley-Kilgore
Copy link
Member

No description provided.

@Riley-Kilgore
Copy link
Member Author

After having these two types unified as Generator, I think it actually may make sense to have two separate prelude types Fuzzer and Sampler. The changes involved are minimal either way, but it seems to make the Fuzzer API somewhat uncomfortable if we are leveraging Generator due to the extra (and unused) parameter.

A lot of users won't be exposed to this, as they will simply use the pre-defined Fuzzers in the Fuzz library, but for users crafting their own Fuzzers, do we want to introduce breaking changes?

@@ -182,6 +183,7 @@ impl fmt::Display for Token {
Token::Once => "once",
Token::Validator => "validator",
Token::Via => "via",
Token::Benchmark => "benchmark",
Copy link
Member

Choose a reason for hiding this comment

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

Let's call it bench; not so much because it's shorter but because it's quite often used in benchmarking terminology (bench refer to the individual tests of a benchmark, which aggregates all benches).

pub name: String,
pub on_test_failure: OnTestFailure,
pub program: Program<Name>,
pub fuzzer: Fuzzer<Name>,
Copy link
Member

Choose a reason for hiding this comment

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

That last one is suspicious? It ought not to be a Fuzzer here I believe.

Comment on lines 533 to 544
pub enum Prng {
Seeded { choices: Vec<u8>, uplc: PlutusData },
Replayed { choices: Vec<u8>, uplc: PlutusData },
Seeded {
choices: Vec<u8>,
uplc: PlutusData,
iteration: usize,
},
Replayed {
choices: Vec<u8>,
uplc: PlutusData,
iteration: usize,
},
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is changing the PRNG necessary? I don't think that it is right? The size parameter should be holistic for an entire PRNG and influence all random choices down the line.

@KtorZ
Copy link
Member

KtorZ commented Dec 17, 2024

A lot of users won't be exposed to this, as they will simply use the pre-defined Fuzzers in the Fuzz library, but for users crafting their own Fuzzers, do we want to introduce breaking changes?

That'll certainly require quite a lot of change in the Fuzz library, which I think we can avoid by making the context be provided as a closure instead of passed as an extra parameter. So, fundamentally, have:

Sampler<a> = fn(Int) -> Fuzzer<a>

instead of

Fuzzer<a> = fn(Void, PRNG) -> Option<(PRNG, a)>
Sampler<a> = fn(Int, PRNG) -> Option<(PRNG, a)>

@Riley-Kilgore Riley-Kilgore changed the title Add Benchmarking command with CSV output; Add ScaledFuzzer to prelude and PBT runner Added Benchmarking; Added Sampler to Prelude for Benchmarking Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants