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

Implement Zstandard streaming decompression #47

Merged
merged 12 commits into from
Aug 14, 2024

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Aug 9, 2024

The output of ZSTD_getFrameContentSize is now checked.

If ZSTD_CONTENTSIZE_UNKNOWN, then we try to stream decompress into a 1 MiB output buffer
that grows in 1 MiB 128 KiB increments

We use WebAssembly.Exception to report errors.

Fix #46

@manzt
Copy link
Owner

manzt commented Aug 13, 2024

Is this ready for review?

@@ -49,7 +49,7 @@ cd ../../../
${OPTIMIZE} \
-I "$CODEC_DIR/lib" \
--closure 1 \
-fwasm-exceptions
-fwasm-exceptions \
Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I see you did use wasm exceptions!

Copy link
Owner

Choose a reason for hiding this comment

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

I believe this is listed as "legacy" now, but the alternative proposal isn't supported in any runtimme at all so I'm good with using for now: https://webassembly.org/features/

@mkitti
Copy link
Contributor Author

mkitti commented Aug 13, 2024

Is this ready for review?

Almost. I got a few notes from Janelia Scientific Computing Code Review this morning, so I have a few changes to make this evening.

I also think a little more work is needed to catch the exceptions properly.

I'm slightly unclear which exception handling feature I'm using on that chart.

@mkitti mkitti marked this pull request as ready for review August 14, 2024 07:50
@mkitti
Copy link
Contributor Author

mkitti commented Aug 14, 2024

According to Mozilla, all major browsers have supported this since 2021 or 2022.

https://developer.mozilla.org/en-US/docs/WebAssembly/JavaScript_interface/Exception#see_also

image

Copy link
Owner

@manzt manzt left a comment

Choose a reason for hiding this comment

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

Awesome, just some minor comments! Thanks for looking in to the exceptions and the nice tests!

src/zstd.ts Show resolved Hide resolved
codecs/zstd/zstd_codec.cpp Outdated Show resolved Hide resolved
codecs/zstd/zstd_codec.cpp Outdated Show resolved Hide resolved
@manzt manzt merged commit c4fe1f5 into manzt:main Aug 14, 2024
4 checks passed
manzt pushed a commit that referenced this pull request Aug 14, 2024
* Initial attempt at implementing Zstandard streaming decompression

* Drop Node 16 due to exeptions support, bump setup-node to v4

* Add streaming decompression tests for streaming compression

* Apply prettier

* Add EXPORT_EXCEPTION_HANDLING_HELPERS

* Code clean up

* Fix typo in build.sh

* Free decompression stream when throwing or exiting

* Throw error if there is additional input data after decompressing frame

* Run prettier on test/zstd.test.js

* Update with recommendations from Janelia SciCompSoft code review

* Update codecs/zstd/zstd_codec.cpp

* Add changeset
@manzt
Copy link
Owner

manzt commented Aug 14, 2024

Thanks! Released in v0.3.2 https://github.com/manzt/numcodecs.js/releases/tag/v0.3.2

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.

The returned value from ZSTD_getFrameContentSize is not checked.
2 participants