Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add read_adapter to avoid dynamic dispatch #1267

Merged
merged 2 commits into from
Dec 7, 2022
Merged

Add read_adapter to avoid dynamic dispatch #1267

merged 2 commits into from
Dec 7, 2022

Conversation

SUPERCILEX
Copy link
Contributor

No description provided.

@@ -470,13 +477,45 @@ impl<R: RngCore + ?Sized> RngCore for Box<R> {
}

#[cfg(feature = "std")]
#[deprecated = "Use rng.read_adapter() instead."]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there are any cases where you couldn't use the read_adapter?

@dhardy dhardy added the B-API Breakage: API label Nov 17, 2022
@dhardy
Copy link
Member

dhardy commented Nov 17, 2022

Can you give an example where this improves benchmark results?

@SUPERCILEX
Copy link
Contributor Author

I forgot to mention that using dyn is also just plain annoying: &mut (&mut rand as &mut dyn RngCore).take(100_000) is just super ugly compared to &mut rand.read_adapter().take(100_000). Using dyn also defaults to a static lifetime which makes life more difficult when passing stuff around. So basically there are strong pros to the read_adapter approach beyond perf.


group.bench_function("dyn-on", |b| {
    b.iter_with_setup(
        || Xoshiro256PlusPlus::seed_from_u64(42),
        |mut rand| {
            io::copy(
                &mut (&mut rand as &mut dyn RngCore).take(100),
                &mut io::sink(),
            )
        },
    );
});
group.bench_function("dyn-off", |b| {
    b.iter_with_setup(
        || Xoshiro256PlusPlus::seed_from_u64(42),
        |mut rand| io::copy(&mut rand.read_adapter().take(100), &mut io::sink()),
    );
});
bytes_generate/dyn-on   time:   [49.346 ns 49.566 ns 49.829 ns]
bytes_generate/dyn-off  time:   [17.053 ns 17.096 ns 17.139 ns]

This was referenced Nov 23, 2022
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@dhardy dhardy merged commit 0dddc2c into rust-random:master Dec 7, 2022
@SUPERCILEX
Copy link
Contributor Author

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
B-API Breakage: API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants