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

replace Box<dyn> with enum #2

Closed
brentp opened this issue Jul 7, 2023 · 1 comment
Closed

replace Box<dyn> with enum #2

brentp opened this issue Jul 7, 2023 · 1 comment

Comments

@brentp
Copy link
Member

brentp commented Jul 7, 2023

We take a substantial performance hit from Boxing everything. We must Box because we are passing around Traits (VCF::Variant, Bam::Record, etc). This means that the dynamic dispatch is done twice since the Box dispatches to the underlying Trait which dispatches to the concrete struct.
https://github.com/brentp/bedder-rs/blob/d051b6520401caddc81a05364838c9065dcb8ee5/src/position.rs#L58-L75

We can use this crate: https://crates.io/crates/enum_dispatch to get static dispatch but we must provide the enum anyway.

Using enum_dispatch or the actual enum would mean that a lib user could not add a new file type (PositionedIterator) without changing the core library--updating the main enum and all code that matches on that enum. (Unless the new filed type used an existing Positioned type)

We can also implement as enums directly. More info on the problem/setup here:
https://bennett.dev/dont-use-boxed-trait-objects-for-struct-internals/

If we do have the enum, a downside is that every Positioned will take the space of the largest item in the Enum.
This is probably OK as memory will only be a problem for huuuge query intervals and we are using Rc, not copying; but it is still a consideration.

The example code shows how this appears when using the API: https://github.com/brentp/bedder-rs/blob/4f106838fb3048430c35796b641cfbc1afd48054/examples/intersect_bed_count.rs#L43-L58

@brentp
Copy link
Member Author

brentp commented Jul 27, 2023

This is implemented in main

@brentp brentp closed this as completed Jul 27, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant