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

[RFC] Update jemalloc to 5.3RC #33057

Merged
merged 23 commits into from
Feb 20, 2022

Conversation

azat
Copy link
Collaborator

@azat azat commented Dec 22, 2021

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Use new jemalloc 5.3RC release

Details:

  • Performance test are OK
  • jemalloc code should be cleaner (also I did some refactoring of generated headers)
  • No more custom patches for jemalloc, pure upstream

Refs:

Checklist:

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Dec 22, 2021
@azat
Copy link
Collaborator Author

azat commented Feb 9, 2022

Performance Comparison (actions) [1/4] — 1 faster, 3 slower, 2 unstable
Details
Performance Comparison (actions) [2/4] — 7 errors, 4 faster, 1 slower, 7 unstable
Details
Performance Comparison (actions) [3/4] — 2 faster, 7 slower, 2 unstable
Details
Performance Comparison (actions) [4/4] — 8 errors, 1 too long, 1 faster, 2 slower, 7 unstable
Details

No significant changes in jemalloc related metrics.

@azat azat changed the title Try new jemalloc from dev branch (for now only linux x86_64) Update jemalloc to 5.3RC Feb 9, 2022
@azat azat changed the title Update jemalloc to 5.3RC [RFC] Update jemalloc to 5.3RC Feb 9, 2022
@azat azat marked this pull request as ready for review February 9, 2022 10:05
@azat azat force-pushed the jemalloc-dev-branch branch 3 times, most recently from 6482a1c to 19216f4 Compare February 9, 2022 16:25
@azat azat marked this pull request as draft February 9, 2022 19:02
@azat azat force-pushed the jemalloc-dev-branch branch 3 times, most recently from b9fafb7 to 6788f3f Compare February 11, 2022 16:36
@azat
Copy link
Collaborator Author

azat commented Feb 12, 2022

@mergify update

@mergify
Copy link
Contributor

mergify bot commented Feb 12, 2022

update

✅ Branch has been successfully updated

Hey, I reacted but my real name is @Mergifyio

And now we can use upstream jemalloc, since all required patches had
been merged into upstream (we have to use fork since there was no new
5.2.x releases).

v2: rebase to include patch for failed assert
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
v2: update jemalloc one more time
v3:
- do not include jemalloc_mangle*.h
- do not change jemalloc.h
- fix for JEMALLOC_NOTHROW/JEMALLOC_SYS_NOTHROW

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
v2: update jemalloc

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Those were just copied from include_linux_x86_64, and replaced x86_64
with the arch.

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
- remove GNU_SOURCE it is done in common CMakeLists.txt
- remove JEMALLOC_OVERRIDE___POSIX_MEMALIGN (there is no need to since
  __posix_memalign() does not exists in linux)

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Note, that there is no need to disable JEMALLOC_PURGE_MADVISE_FREE,
since jemalloc does check in runtime, and ClickHouse already
successfully works w/o this change.

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Generated on osx 10.14

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
…ilds

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@azat azat marked this pull request as ready for review February 17, 2022 18:56
@azat
Copy link
Collaborator Author

azat commented Feb 18, 2022

@alexey-milovidov
Copy link
Member

Now more custom patches for jemalloc, pure upstream

You mean:

"No more custom patches for jemalloc, pure upstream"

@alexey-milovidov
Copy link
Member

I don't like that "Performance Comparison" has errors. We need to fix all of them.

@alexey-milovidov
Copy link
Member

@kitaisreal will fix "Performance Comparison"

@azat
Copy link
Collaborator Author

azat commented Feb 18, 2022

You mean:
"No more custom patches for jemalloc, pure upstream"

Right.

@alexey-milovidov alexey-milovidov merged commit 26d0e54 into ClickHouse:master Feb 20, 2022
@alexey-milovidov alexey-milovidov self-assigned this Feb 20, 2022
@azat azat deleted the jemalloc-dev-branch branch February 22, 2022 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-not-for-changelog This PR should not be mentioned in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants