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.

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

Jump to bottom

Purge jemalloc arenas in case of high memory usage. #40277

Merged
merged 11 commits into from
Sep 20, 2022
Merged

Purge jemalloc arenas in case of high memory usage. #40277

merged 11 commits into from
Sep 20, 2022

Conversation

KochetovNicolai
Copy link
Member

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Another version of #38682
For now, without separate counter for RSS.

@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Aug 16, 2022
@KochetovNicolai
Copy link
Member Author

00725_memory_tracking again :(

azat
azat reviewed Aug 19, 2022
View reviewed changes
src/Common/MemoryTracker.cpp
{
if (free_memory_in_allocator_arenas.exchange(-current_free_memory_in_allocator_arenas))
{
mallctl("arena." STRINGIFY(MALLCTL_ARENAS_ALL) ".purge", nullptr, nullptr, nullptr, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like it definitely deserves some metrics (not suggesting logging because it is memory tracker).
What do you think?

P.S. Now MemoryTracker code looks more and more complicated...

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, should be easy to add.
Now it is more like experimental pr

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now it is more like experimental pr

Well it is merged...

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's bet how soon it would be rolled back :D
My bet that less then in a week.

src/Interpreters/AsynchronousMetrics.cpp

Int64 difference = new_amount - amount;
Int64 difference = rss - free_memory_in_allocator_arenas - amount;

/// Log only if difference is high. This is for convenience. The threshold is arbitrary.
if (difference >= 1048576 || difference <= -1048576)
LOG_TRACE(log,
"MemoryTracking: was {}, peak {}, will set to {} (RSS), difference: {}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe also put free_memory_in_allocator_arenas value into the log?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No message in the log

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, sorry.
Well, this value is still available from system table...

@azat
Copy link
Collaborator

azat commented Aug 27, 2022

BTW right now one can use the following trick to reduce jemalloc memory fragmentation:

clickhouse-benchmark -c 100 --port 19000 -q 'select 1'

This touches almost every thread, and so jemalloc will purge per-cpu arena.

@KochetovNicolai
Copy link
Member Author

#41548

@nikitamikhaylov nikitamikhaylov self-assigned this Sep 20, 2022
@nikitamikhaylov nikitamikhaylov merged commit 9e1bb19 into master Sep 20, 2022
@nikitamikhaylov nikitamikhaylov deleted the purge-arenas-in-jemalloc branch September 20, 2022 21:56
azat
azat reviewed Sep 21, 2022
View reviewed changes
src/Common/MemoryTracker.cpp
{
amount.store(to, std::memory_order_relaxed);
Int64 new_amount = rss_; // - free_memory_in_allocator_arenas_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the comment is irrelevant already

src/Interpreters/AsynchronousMetrics.cpp

Int64 difference = new_amount - amount;
Int64 difference = rss - free_memory_in_allocator_arenas - amount;

/// Log only if difference is high. This is for convenience. The threshold is arbitrary.
if (difference >= 1048576 || difference <= -1048576)
LOG_TRACE(log,
"MemoryTracking: was {}, peak {}, will set to {} (RSS), difference: {}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

No message in the log

src/Common/MemoryTracker.cpp
{
if (free_memory_in_allocator_arenas.exchange(-current_free_memory_in_allocator_arenas))
{
mallctl("arena." STRINGIFY(MALLCTL_ARENAS_ALL) ".purge", nullptr, nullptr, nullptr, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now it is more like experimental pr

Well it is merged...

@amosbird
Copy link
Collaborator

Is it normal that server log is flooded with:

2022.09.28 06:33:34.001433 [ 3133669 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 562.20 MiB, peak 562.21 MiB, will set to 562.20 MiB (RSS), difference: -70.46 MiB
2022.09.28 06:33:35.001639 [ 3133669 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 562.20 MiB, peak 562.21 MiB, will set to 562.20 MiB (RSS), difference: -70.45 MiB
2022.09.28 06:33:36.001455 [ 3133669 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 562.20 MiB, peak 562.21 MiB, will set to 562.21 MiB (RSS), difference: -70.44 MiB
2022.09.28 06:33:37.001656 [ 3133669 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 562.21 MiB, peak 562.21 MiB, will set to 562.21 MiB (RSS), difference: -70.45 MiB
2022.09.28 06:33:38.001526 [ 3133669 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 562.21 MiB, peak 562.21 MiB, will set to 562.21 MiB (RSS), difference: -70.45 MiB
2022.09.28 06:33:39.001702 [ 3133669 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 562.21 MiB, peak 562.21 MiB, will set to 562.21 MiB (RSS), difference: -70.45 MiB
2022.09.28 06:33:40.001531 [ 3133669 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 562.21 MiB, peak 562.21 MiB, will set to 562.21 MiB (RSS), difference: -70.45 MiB

@azat azat mentioned this pull request Oct 22, 2022
Merged
@azat
Copy link
Collaborator

azat commented Oct 22, 2022

Is it normal that server log is flooded with:

#42582 should address this

@azat azat mentioned this pull request Jul 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azat azat azat left review comments

@nikitamikhaylov nikitamikhaylov nikitamikhaylov approved these changes

Assignees

@nikitamikhaylov nikitamikhaylov

Labels
pr-not-for-changelog This PR should not be mentioned in the changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@KochetovNicolai @azat @amosbird @nikitamikhaylov @robot-ch-test-poll1