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

feature-gate parallel (de)compression #201

Closed
wants to merge 3 commits into from

Conversation

dakom
Copy link

@dakom dakom commented Feb 28, 2023

  • feature-gates parallel (de)compression
  • doesn't change current API (enables by default)
  • fixes WASM support #200

@johannesvollmer
Copy link
Owner

johannesvollmer commented Feb 28, 2023

thanks for taking the time! looks reasonable, although I'd refactor it later some time to reduce the number of conditionals, maybe by moving all the parallel stuff into one module and cfg-ing that module as a whole. not now, though. i think we can merge this, I'll review it soon

one thing i want to see is some kind of test in the CI that checks whether the library actually compiles when this flag is active, otherwise someone might break the flag later without noticing

@johannesvollmer
Copy link
Owner

johannesvollmer commented Feb 28, 2023

here's the CI pipeline. it should be fairly easy to add a job for compilation and test execution with the flag. are you interested in adding that?

@dakom
Copy link
Author

dakom commented Feb 28, 2023

thanks for the quick response! agreed on all those points :)

Added the CI job

I had to make a small opinionated refactor to get the tests to work, in 8_read_raw_blocks, moving the callback into a closure, but thankfully you had already structured both the sequential and parallel versions to take the same callback, so it was pretty easy

it's late for me - happy to followup more tomorrow ✌️

@dakom
Copy link
Author

dakom commented Mar 1, 2023

looks like they're all green :)

@dakom dakom mentioned this pull request Mar 1, 2023
@johannesvollmer
Copy link
Owner

I can probably merge on the weekend :)

@johannesvollmer
Copy link
Owner

after thinking about it, I would definitely go the route of automatic fallback to sequential decompressions. I'd still accept having the feature flag, but I would first code up the fallback mechanism, as this will probably remove the need for some of the cfg(...) conditions in this pr.

I appreciate your time and the work you put in this. and I would welcome if you still want to merge the changed to the continuous integration. I would like to postpone the feature flag until the fallback is in place though. what are your thoughts?

@dakom
Copy link
Author

dakom commented Mar 6, 2023

Yeah, all good, feel free to close the PR if you prefer :)

The main thing for me was just being able to get wasm to work, and I wrote the PR before I realized that the error could just be avoided by specifying non-parallel

@johannesvollmer
Copy link
Owner

johannesvollmer commented Mar 6, 2023

I'd still appreciate if you added the WASM CI task if you have the time. Just to celebrate your will to contribute at all :) I hope you understand what I mean. And of course for value it would bring to this project, by boosting the confidence for wasm targets

@dakom
Copy link
Author

dakom commented Mar 13, 2023

heya, sorry I was afk last week... reminding myself to circle back to this :)

johannesvollmer added a commit that referenced this pull request Jul 3, 2023
thanks to @dakom for the implementation :)
johannesvollmer added a commit that referenced this pull request Jul 3, 2023
@johannesvollmer
Copy link
Owner

the project will use a different mechanism for controlling whether threading is used, but thanks for the wasm ci code :)

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.

WASM support
2 participants