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

Use V8's zlib #2522

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Use V8's zlib #2522

merged 1 commit into from
Aug 27, 2024

Conversation

npaun
Copy link
Member

@npaun npaun commented Aug 12, 2024

This PR changes the source of zlib to match what V8 is using, so that we don't have version conflicts between workerd and the internal edgeworker repo.

zlib is built from source. The build config is a translation from the BUILD.gn file into bazel syntax.

Note that this is a bit of a different build process from edgeworker, which uses precompiled binaries from v8 for zlib.

@npaun npaun requested review from jasnell and anonrig August 12, 2024 22:40
@npaun npaun requested review from a team as code owners August 12, 2024 22:40
@fhanau
Copy link
Collaborator

fhanau commented Aug 12, 2024

Couple thoughts on this –

  • we could use our own high-performance zlib fork (cloudflare/zlib) here, but I think using Chromium zlib is better for consistency since we use V8 a lot. Chromium zlib also has a number of performance optimizations (in fact some of the decompression optimizations in Cloudflare zlib originate from Chromium zlib) and should work well.
  • I looked into switching a while back but got bogged down by configuring the build to configure SIMD support properly. I think we should do this eventually but it's not required for now – the GN build file is pretty complex and we'd also have to properly document what level of hardware extensions workerd requires. This is best done together with compiling with e.g. x86-64-v3 by default.
  • I don't think we should test for ZLIB_VERNUM at all – in my experience it is not relevant to developers, except when checking if some C-level API that got added in a certain version will be available, or perhaps if a bug fix added in a certain version is added. Either way, we don't depend on zlib being a specific fixed version and shouldn't have to update a test every time the zlib version is incremented.

Change LGTM but it looks like there's SIMD-related compile errors on Windows so building this using Bazel might be more complex

@jasnell
Copy link
Member

jasnell commented Aug 12, 2024

@anonrig ... your experience with simd may be helpful in the longer term effort here

@npaun
Copy link
Member Author

npaun commented Aug 12, 2024

Thanks for the notes @fhanau

I don't care about ZLIB_VERNUM itself, just that there is consistency between what is being used in workerd and edgeworker, so as to eliminate a possible source of trouble.

I'll have a look tomorrow into what is going wrong on Windows. Fingers crossed that it's not too hard to fix.

WORKSPACE Outdated Show resolved Hide resolved
@anonrig
Copy link
Member

anonrig commented Aug 13, 2024

Node.js ZLIB configuration might give you a better understanding for the SIMD related issues. If you couldn't solve this in a reasonable amount of time, I have a Windows machine at home.

https://github.com/anonrig/node/blob/main/deps/zlib/zlib.gyp

@npaun npaun force-pushed the npaun/zlib-from-v8 branch 3 times, most recently from 00128a8 to e1b55cc Compare August 13, 2024 17:26
@npaun npaun requested a review from a team as a code owner August 19, 2024 18:51
@npaun
Copy link
Member Author

npaun commented Aug 19, 2024

May resolve #2540 if we decide to go with this approach.

@npaun npaun linked an issue Aug 19, 2024 that may be closed by this pull request
@npaun npaun force-pushed the npaun/zlib-from-v8 branch 8 times, most recently from d1b9ac5 to d8aea2c Compare August 27, 2024 16:58
@@ -0,0 +1,194 @@
# This config closely follows the original GN build file
Copy link
Member Author

Choose a reason for hiding this comment

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

Note for reviewers: In this file, I try to keep everything as similar as possible to the original BUILD.gn even if it looks strange in bazel

@npaun
Copy link
Member Author

npaun commented Aug 27, 2024

Internal PR passes

@npaun npaun requested a review from anonrig August 27, 2024 17:46
@npaun npaun merged commit 290b54c into main Aug 27, 2024
14 of 15 checks passed
@npaun npaun deleted the npaun/zlib-from-v8 branch August 27, 2024 18:31
@npaun npaun restored the npaun/zlib-from-v8 branch August 27, 2024 18:31
@npaun npaun deleted the npaun/zlib-from-v8 branch August 27, 2024 18:31
@fhanau
Copy link
Collaborator

fhanau commented Aug 28, 2024

Thank you for doing this! Should certainly improve performance in use cases where developers use compression APIs within workerd a lot.

"@platforms//cpu:x86_64": ["-mssse3"],
"//conditions:default": [],
}),
defines = select({
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should really be local_defines – defines are added to every target that depends on zlib otherwise, which is really polluting the global defines namespace and unnecessarily making the command line of workerd longer. https://bazel.build/reference/be/c-cpp#cc_library.local_defines

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree; the main reason I left it all as define is to match BUILD.gn. I can take a crack at limiting the scope in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's also another nuisance: I'm using defines to avoid nested selects which don't work in bazel. I am actually not entirely sure how to get around this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great question! We use selects.config_setting_group (https://bazel.build/docs/configurable-attributes#and-chaining) for this. Started with a PR on converting those.

"crc32_simd.h",
"crc_folding.c",
],
copts = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

This adds a requirement on ISA extensions that we didn't have before – IMHO this is reasonable since these extensions have been around for a long time but we need to document it.
If we're doing that it's reasonable to define the same set of extensions for the entire build and remove the individual flags from here (I agree that we should follow the structure of BUILD.gn for the individual targets but to make the file less complex we should not have to define ISA extension flags for each target). I will prepare a PR for this.

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.

🐛 Bug Report — Runtime APIs
4 participants