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

fix: avoid duplicate symbols #2

Merged
merged 14 commits into from
Nov 24, 2023
Merged

fix: avoid duplicate symbols #2

merged 14 commits into from
Nov 24, 2023

Conversation

dignifiedquire
Copy link
Contributor

No description provided.

@qti3e
Copy link

qti3e commented Nov 19, 2023

Coming here from BLAKE3-team/BLAKE3#351, Just wanted to point out that what we ended up doing at our fork (fleek-blake3) was to import the platform from the upstream, instead of renaming the symbols. This can help with the final binary size since it does not include multiple symbols doing the same thing. I thought it might be helpful to point out here. Sorry if not. But check out the Cargo.toml modifications we had to do and the re-export of platform.

BLAKE3-team/BLAKE3@master...fleek-network:BLAKE3:master#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542

@flub
Copy link
Contributor

flub commented Nov 24, 2023

Coming here from BLAKE3-team/BLAKE3#351, Just wanted to point out that what we ended up doing at our fork (fleek-blake3) was to import the platform from the upstream, instead of renaming the symbols. This can help with the final binary size since it does not include multiple symbols doing the same thing. I thought it might be helpful to point out here. Sorry if not. But check out the Cargo.toml modifications we had to do and the re-export of platform.

BLAKE3-team/BLAKE3@master...fleek-network:BLAKE3:master#diff-2e9d962a08321605940b5a657135052fbcef87b5e360662bb527c96d9a615542

IIUC you are never directly calling the original blake3 crate and instead rely on these symbols, which are not public for rust, being exposed by the linker anyway so they are sneakily used from the original crate.

Interesting approach, it does indeed save some duplication. I'm not sure if it's more of a hack or not. I'm not really sure if I have a preference on either approach.

@qti3e
Copy link

qti3e commented Nov 24, 2023

We actually do make calls to the original crate. The platform mod is #[doc(hidden)], so it means anyone has access to blake3::platform (it's just not in the docs), and because of that, our fork actually duplicates the rust logics on top of the bindings (like the Hasher struct or ChunkState), but since we swap out our fork's platform mod with the actual platform module from upstream we will not duplicate the assembly symbols.

Notice how in the diff we have removed all of the binding files (ffi_avx2.rs, ffi_avx512.rs, ffi_*.rs) and the build.rs and this particular line in lib.rs:

/// Undocumented and unstable, for benchmarks only.
#[doc(hidden)]
- pub mod platform;
+ pub use blake3::platform;

The next trick is to forward your crate features to the upstream, for that checkout Cargo.toml and how we have this pattern on each feature:

neon = ["blake3/neon"]
xxx = [..., "blake3/xxx"]

@flub flub merged commit f644fb2 into master Nov 24, 2023
88 of 98 checks passed
@rklaehn
Copy link
Collaborator

rklaehn commented Nov 29, 2023

We actually do make calls to the original crate. The platform mod is #[doc(hidden)], so it means anyone has access to blake3::platform (it's just not in the docs), and because of that, our fork actually duplicates the rust logics on top of the bindings (like the Hasher struct or ChunkState), but since we swap out our fork's platform mod with the actual platform module from upstream we will not duplicate the assembly symbols.

Notice how in the diff we have removed all of the binding files (ffi_avx2.rs, ffi_avx512.rs, ffi_*.rs) and the build.rs and this particular line in lib.rs:

/// Undocumented and unstable, for benchmarks only.
#[doc(hidden)]
- pub mod platform;
+ pub use blake3::platform;

The next trick is to forward your crate features to the upstream, for that checkout Cargo.toml and how we have this pattern on each feature:

neon = ["blake3/neon"]
xxx = [..., "blake3/xxx"]

I like this approach. However, I guess we just release what we currently have to alleviate the duplicate symbol pain in the short term, and hope for the blake3-guts crate. If that takes longer, we will probably copy your approach.

@rklaehn rklaehn deleted the fix-avoid-symbols branch November 29, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants