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

Removed dead code from a macro in zmalloc.cpp #242

Merged
merged 1 commit into from
Oct 4, 2020

Conversation

hbina
Copy link
Contributor

@hbina hbina commented Oct 3, 2020

I think the compiler would have removed this no-op anyways but it
definitely wasted me some 30 minutes on this :(

I was hoping I could remove the branch through some bit-hacking but
apparently its dead code :). Oh well, its 30 minutes of refreshing
bit hacking.

  1. Is there a progress on this mimalloc request in the works?

  2. What do you think of using std::atomic<std::size_t> to replace the "variable + mutex" pattern?

Signed-off-by: Hanif Bin Ariffin hanif.ariffin.4326@gmail.com

I think the compiler would have removed this no-op anyways but it
definitely wasted me some 30 minutes on this :(

I was hoping I could remove the branch through some bit-hacking but
apparently its dead code :). Oh well, its 30 minutes of refreshing
bit hacking.

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
@JohnSully
Copy link
Collaborator

JohnSully commented Oct 4, 2020

I'm not sure that's dead code. It looks like its rounding up to the nearest 4 bytes. Necessary because the allocator will always ensure at least a 4-byte alignment so if you allocate 3 bytes your getting 4.

@hbina
Copy link
Contributor Author

hbina commented Oct 4, 2020

its dead code because while its modifying _n, only __n is used. Naming is hard.
Also, if you are correct, then this should not be the place for that alignment calculations to happen because this is only for statistics. Such calculations should be performed prior to any allocation.

@JohnSully
Copy link
Collaborator

Hmm indeed. Well I don't see a need to change behavior now so lets take this change.

@JohnSully
Copy link
Collaborator

I wouldn't switch right away to std::atomic as it defaults all operations to SEQ_CST memory order which has been a perf issue elsewhere. Lets just take this simplified change for now.

@JohnSully JohnSully merged commit 8171e6d into Snapchat:unstable Oct 4, 2020
@hbina hbina deleted the removed_dead_code_zmalloc branch October 4, 2020 01:10
JohnSully pushed a commit that referenced this pull request Feb 1, 2024
* TLS cname loadshedding ignore list
This pull request was closed.
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.

2 participants