Skip to content

zlib: reduce code duplication #57810

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −5
Open

zlib: reduce code duplication #57810

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/node_zlib.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,9 +607,11 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork {
return AllocForBrotli(data, real_size);
}

static constexpr size_t reserveSizeAndAlign =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static constexpr size_t reserveSizeAndAlign =
static constexpr size_t kReserveSizeAndAlign =

Not 100% sure but I think this doesn't fit the code style, it seems inconsistent already in the codebase
Quite some places use a k prefix, others use define style (e.g. RESERVER_SIZE_AND_ALIGN).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that sounds like pure evil, are you suggestion to make it a #define in a c++ project?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn't mean to convert it to a define, constexpr is perfectly fine and the way to go.

It's just about nitpicking about the name. Unfortunatelly cpp-style-guide doesn't clarify this.

Looking into code I found several places using the k prefix (e.g. here), and some using ALL_UPPER_CASE (e.g. here).

Personally I prefer the proposed k prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, I guess I can rename it to reserveSizeAndAlign_, although rather ugly, that seems to comply with the code standard / general implementations. I doubt I have ever seen a k though, unless in kilo. Why would you prefer a k there?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know where the k prefix comes from but it is used at quite some places in this repo for constants. Therefore I see it as helpful as it jumps into my mind that this is a constant. Would I go for k prefix in my personal repo: no - but thats a different topic.
The casing you use now is used nowhere in this repo for constants.

Aynhow, all I'm asking is to try to follow the existing style8s). I guess kReserveSizeAndAlign, RESERVE_SIZE_AND_ALIGN, reserver_size_and_align (and maybe more) follow exisiting styles.

std::max(sizeof(size_t), alignof(max_align_t));
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just always alignof(max_align_t) anyway?

Copy link
Contributor Author

@jhofstee jhofstee Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

no, if you don't care about unaligned accesses you could set it to 1. There is no architecture actually doing that though. But this is just addressing a comment made by @Flarna.

Copy link
Contributor Author

@jhofstee jhofstee Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

see #57727 (comment) as well, this just removes a double definition of a constant, there is nothing special about this.

Copy link
Member

@addaleax addaleax Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm confused, you're saying "no" here but the comment you linked seems to unambiguously answer my question with "yes"? 🙂

Either way, I don't really care, but I'm not sure that this code is clearer because when reading it it really should raise a question of "why are we defining this constant as the maximum of two values where one is always clearly larger than the other"

Copy link
Member

Choose a reason for hiding this comment

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

I'm not aware of any platform where alignof(max_align_t) is smaller then sizeof(size_t) as most hardware prefers aligned data.
But to my understanding C++ standard allows alignof(max_align_t) == 1 but the space needed here is sizeof(size_t) so take the max.

Copy link
Contributor Author

@jhofstee jhofstee Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax Just make a large struct or class and you can easily check that the alignment of it is less then its size. This code is simply doing the right thing, it will make sure there is enough space and it is still aligned. In theory alignof(max_align_t) can be less than sizeof(size_t). In practice it won't, but the compiler will optimize it out anyway.


static void* AllocForBrotli(void* data, size_t size) {
constexpr size_t offset = std::max(sizeof(size_t), alignof(max_align_t));
size += offset;
size += reserveSizeAndAlign;
CompressionStream* ctx = static_cast<CompressionStream*>(data);
char* memory = UncheckedMalloc(size);
if (memory == nullptr) [[unlikely]] {
Expand All @@ -618,16 +620,15 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork {
*reinterpret_cast<size_t*>(memory) = size;
ctx->unreported_allocations_.fetch_add(size,
std::memory_order_relaxed);
return memory + offset;
return memory + reserveSizeAndAlign;
}

static void FreeForZlib(void* data, void* pointer) {
if (pointer == nullptr) [[unlikely]] {
return;
}
CompressionStream* ctx = static_cast<CompressionStream*>(data);
constexpr size_t offset = std::max(sizeof(size_t), alignof(max_align_t));
char* real_pointer = static_cast<char*>(pointer) - offset;
char* real_pointer = static_cast<char*>(pointer) - reserveSizeAndAlign;
size_t real_size = *reinterpret_cast<size_t*>(real_pointer);
ctx->unreported_allocations_.fetch_sub(real_size,
std::memory_order_relaxed);
Expand Down
Loading