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

Enhancement: opt-in to C-based (instead of asm) implementation #2

Closed
pnkfelix opened this issue Aug 3, 2023 · 3 comments
Closed

Enhancement: opt-in to C-based (instead of asm) implementation #2

pnkfelix opened this issue Aug 3, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@pnkfelix
Copy link
Owner

pnkfelix commented Aug 3, 2023

I want to prototype a variant of the crate that totally avoids inline asm and uses ("portable"?) C instead.

I ended up posting this crate without that in place because I wasn't sure the best architecture to use for that C dependency. (E.g. should it be a separate crate? Or can I make it a separate file that is conditionally-compiled?)

@pnkfelix pnkfelix added the enhancement New feature or request label Aug 3, 2023
This was referenced Aug 3, 2023
@pnkfelix
Copy link
Owner Author

pnkfelix commented Aug 3, 2023

Note that the current implementation leverages generic code by generating distinct asm blocks for each concrete callback F.

It wouldn't be reasonable to do that via C code. But we don't have to; we can make the generic C version take the closure environment and the closure code pointer as explicit arguments, and then do the call itself. Its hypothetically a performance hit to do that, but my gut tells me that its not a noticeable performance hit: either you have LTO turned on, in which case I hope you'd see enough inlining to boil away the indirection, or you have LTO turned off, in which case you won't see any cross-language inlining anyway and I doubt you'd care about an indirect vs direct call.

@jeff-davis
Copy link
Contributor

This seems like a reasonable thing to do. To me it would make sense to fall back to C for unrecognized architectures, or to allow forcing the C version with a feature flag. That would be a nice safety net in case we discover an obscure bug with either the C version or the asm version.

@pnkfelix
Copy link
Owner Author

Addressed by #21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants