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

zlib one-shot methods #2533

Merged
merged 1 commit into from
Aug 26, 2024
Merged

zlib one-shot methods #2533

merged 1 commit into from
Aug 26, 2024

Conversation

npaun
Copy link
Member

@npaun npaun commented Aug 14, 2024

  • Reuses ZlibContext from @anonrig's implementation of the stream-based API.
  • Sends the entire input buffer at once, and uses only the finishFlush flag, like Node's does.
  • Uses GrowableBuffer to collect the output as it's generated. This is basically kj::Vector, but it can be limited to a maximum size and adds some methods to make usage as a buffer more ergonomic.

@npaun npaun force-pushed the npaun/zlib-inflate-sync branch 13 times, most recently from 62e82ed to bd9fee7 Compare August 22, 2024 17:39
src/node/zlib.ts Outdated Show resolved Hide resolved
src/workerd/api/node/tests/zlib-nodejs-test.js Outdated Show resolved Hide resolved
src/workerd/api/node/tests/zlib-nodejs-test.js Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.c++ Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.c++ Outdated Show resolved Hide resolved
src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
src/node/internal/zlib.d.ts Show resolved Hide resolved
@npaun npaun force-pushed the npaun/zlib-inflate-sync branch 2 times, most recently from 781b791 to e3a9696 Compare August 22, 2024 22:02
@npaun
Copy link
Member Author

npaun commented Aug 22, 2024

@jasnell Do we want to respect chunkSize for output even though these are one-shot methods? I think we could theoretically ignore it and pick something larger for performance, but that's not how Node does it.

src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.h Outdated Show resolved Hide resolved
@npaun npaun requested a review from a team as a code owner August 23, 2024 18:25
@npaun npaun force-pushed the npaun/zlib-inflate-sync branch 3 times, most recently from e3a9696 to cb724a4 Compare August 23, 2024 20:24
@npaun npaun requested a review from anonrig August 23, 2024 20:27
@@ -41,19 +285,6 @@ Object.defineProperties(
)
);

export function crc32(
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move this function change to a different pull-request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Normally I'm in favour of splitting PRs into small chunks, however, this time I think the changes are OK to go together, because I'm reusing the input source stuff between zlibsync and crc32sync

import { Zlib } from 'node-internal:internal_zlib_base';

type CompressCallback = (error: Error | null, result: Buffer | null) => void;
Copy link
Member

Choose a reason for hiding this comment

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

This type is wrong. Technically this supports (null, null), but it is not possible.

Suggested change
type CompressCallback = (error: Error | null, result: Buffer | null) => void;
type CompressCallback = ((error: Error, result: null) => void) | ((null, result: Buffer) => void)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that's much better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure the new type is right either. To me it says "a function that handles errors" OR "a function that handles results". You can't pass in a function that handles both with that type. FWIW, DefinitelyTyped uses a type like the one I had originally.

src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.c++ Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.c++ Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.c++ Show resolved Hide resolved
src/workerd/api/node/zlib-util.c++ Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.h Show resolved Hide resolved
@npaun
Copy link
Member Author

npaun commented Aug 26, 2024

Addressed the latest round of suggestions; thanks everyone. Please have another look.

src/node/internal/internal_zlib.ts Outdated Show resolved Hide resolved
src/node/internal/zlib.d.ts Outdated Show resolved Hide resolved
src/workerd/api/node/zlib-util.c++ Outdated Show resolved Hide resolved
@npaun npaun merged commit 2c6af65 into main Aug 26, 2024
9 of 10 checks passed
@npaun npaun deleted the npaun/zlib-inflate-sync branch August 26, 2024 20:26
@anonrig
Copy link
Member

anonrig commented Aug 26, 2024

@npaun just fyi: merged commit has WIP: prefix on it.

npaun added a commit that referenced this pull request Aug 26, 2024
npaun added a commit that referenced this pull request Aug 26, 2024
@npaun npaun mentioned this pull request Aug 27, 2024
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.

5 participants