8292989: Avoid dynamic memory in AsyncLogWriter #1630

navyxliu · 2023-07-28T08:18:46Z

This patch is NOT a clean backport. The logic part is exactly the same, but I have to adjust code in the following parts:

change from KVHashtable to ResourceHashtable because we want to use resourceArea instead of C_HEAP.
rewrite lambda expression using Functor.

Testing:
jdk-test1 tests including gtest.
Manually check there's no malloc/free in runtime.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
JDK-8292989 needs maintainer approval

Issue

JDK-8292989: Avoid dynamic memory in AsyncLogWriter (Enhancement - P4)

Reviewers

Paul Hohensee (@phohensee - Reviewer) ⚠️ Review applies to 4511c52f

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/1630/head:pull/1630
$ git checkout pull/1630

Update a local copy of the PR:
$ git checkout pull/1630
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/1630/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1630

View PR using the GUI difftool:
$ git pr show -t 1630

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/1630.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-07-28T08:19:09Z

👋 Welcome back xliu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-07-28T08:20:34Z

This backport pull request has now been updated with issue from the original commit.

mlbridge · 2023-07-28T08:25:05Z

Webrevs

01: Full - Incremental (a0bf39dd)
00: Full (4511c52f)

openjdk · 2023-07-28T14:48:41Z

@navyxliu This change is no longer ready for integration - check the PR body for details.

phohensee · 2023-07-28T14:49:52Z

windows-aarch64 build failure running adlc appears unrelated.

shipilev · 2023-08-01T18:15:49Z

Before I review the change, should https://bugs.openjdk.org/browse/JDK-8294075 be backported as well? Is JDK-8294075 caused by this change?

shipilev

Looked through the code, I have comments.

shipilev · 2023-08-01T19:30:35Z

src/hotspot/share/logging/logAsyncWriter.hpp

+  using AsyncLogMap = ResourceHashtable<LogFileStreamOutput*, uint32_t, primitive_hash<LogFileStreamOutput*>,
+                                        primitive_equals<LogFileStreamOutput*>, 17,
+                                        ResourceObj::C_HEAP, mtLogging>;


These arguments should be on the new lines each, I think? This would match the upstream code better.

hi, @shipilev
Do we have linter rules for this?

even in tip of jdk, I still can find code like this:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/classfile/classLoaderStats.hpp#L114

shipilev · 2023-08-01T19:33:39Z

src/hotspot/share/utilities/resourceHash.hpp

@@ -184,11 +184,20 @@ class ResourceHashtable : public ResourceObj {
  // the iteration is cancelled.
  template<class ITER>
  void iterate(ITER* iter) const {
+    auto function = [&] (K& k, V& v) {


Can we avoid doing this by making a named struct implementing do_entry for every use? I realize that would deviate the uses from the upstream, but we would not be changing the shared code, which would be safer for generic code that does not use any of async logging and uses older toolchains.

This hunk effectively backports https://bugs.openjdk.org/browse/JDK-8276789, which seems to imply the need to check this works well with different toolchains.

yes, I can rewrite lambdas using old-style functor.

to backport JDK-8276789, we also need to backport bucket_at(), table()/table_size() beforehand. I take your advice. my approach skips more verification and it's not good. I will use functor.

navyxliu · 2023-08-01T20:03:20Z

Before I review the change, should https://bugs.openjdk.org/browse/JDK-8294075 be backported as well? Is JDK-8294075 caused by this change?

we found that it doesn't work. we still have intermittent failure.
https://bugs.openjdk.org/browse/JDK-8309067

tstuefe · 2023-08-05T06:17:33Z

Is it necessary to downport this patch? On its own, it does not sound that important, especially if we downport the glibc autotrimmer.

navyxliu · 2023-08-07T05:38:22Z

Hi, Thomas,

Thanks you for the comments. I was told that it is better reducing memory footprint of UL in jdk17u, a LTS Java. May I learn more about glibc autotrim? Is it a glibc feature or a new feature of hotspot?

I found JDK-8268893 on JBS. It itself is a very useful feature! It is not an automatic trimmer, is it?

tstuefe · 2023-08-07T09:20:49Z

Hi Xin :)

Hi, Thomas,

Thanks you for the comments. I was told that it is better reducing memory footprint of UL in jdk17u, a LTS Java.

Interesting, I'm curious about the effectiveness of your patch. Do you see good results? I would have thought that a lot of these interleaving memory requests are just taken from and handed back into fastbins or tcache, and therefore don't make the arena grow. Also, the glibc attempts to reclaim memory automatically are quite limited (see PR description openjdk/jdk#14781 (comment))

May I learn more about glibc autotrim? Is it a glibc feature or a new feature of hotspot?

I found JDK-8268893 on JBS. It itself is a very useful feature! It is not an automatic trimmer, is it?

Its https://bugs.openjdk.org/browse/JDK-8293114. You have to manually enable it, it is still marked as experimental. Also not in 17 yet, but Aleksey is preparing things to get it down quickly once it has baked in head a bit.

Essentially, what the trimmer does is run periodically a malloc_trim() operation. malloc_trim() will reclaim memory that has been free'd and that is in page granularity (so, it will also reclaim memory in the middle of an arena).

Cheers, Thomas

shipilev · 2023-08-07T09:48:55Z

Is it necessary to downport this patch? On its own, it does not sound that important, especially if we downport the glibc autotrimmer.

I think that autotrimmer is a mitigation strategy for oversubscription, rather than a fix. it also does not resolve fragmentation issues, if there are surviving mallocs between temporarly async logging mallocs. In other words, it is more compelling to avoid the issue instead of relying on auto-trimmer. We use a lot of -Xlog:async in production, and this would give us additional safety against memory fragmentation/oversubscription at least with glibc.

navyxliu · 2023-08-09T18:51:35Z

hi, @tstuefe,

I think we need both of them. This patch refrains from using any malloc/free in the first place. It also helps avoid the fragmentation issue. Autotrimmer is a general solution for glibc overprovision.

Should we still consider to include this patch in jdk17u?

tstuefe · 2023-08-10T04:59:46Z

hi, @tstuefe,

I think we need both of them. This patch refrains from using any malloc/free in the first place. It also helps avoid the fragmentation issue. Autotrimmer is a general solution for glibc overprovision.

Should we still consider to include this patch in jdk17u?

I don't want to block this patch, so sure. I just would have liked to know what the strategy brings in terms of RSS. Plain curiousity.

shipilev

OK, this looks good, but I think we should fix/backport these two before backporting this one to 17u:
https://bugs.openjdk.org/browse/JDK-8297302 (fixed, should be backported)
https://bugs.openjdk.org/browse/JDK-8309067 (not fixed, should be dealt with first)

src/hotspot/share/logging/logAsyncWriter.cpp

navyxliu · 2023-08-10T17:40:43Z

https://bugs.openjdk.org/browse/JDK-8309067 (not fixed, should be dealt with first)

okay. I will work on JDK-8309067

openjdk · 2023-09-21T18:37:23Z

⚠️ @navyxliu This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

bridgekeeper · 2023-10-19T18:46:31Z

@navyxliu This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2023-11-16T23:06:37Z

@navyxliu This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Backport bf79f99c0c61fc36b80cd8550e5c70e158338ef4

4511c52

openjdk bot changed the title ~~Backport bf79f99c0c61fc36b80cd8550e5c70e158338ef4~~ 8292989: Avoid dynamic memory in AsyncLogWriter Jul 28, 2023

openjdk bot added backport rfr Pull request is ready for review labels Jul 28, 2023

phohensee approved these changes Jul 28, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 28, 2023

shipilev reviewed Aug 1, 2023

View reviewed changes

Revert resourceHash.hpp and use functor instead of C++ lambda.

a0bf39d

shipilev reviewed Aug 10, 2023

View reviewed changes

src/hotspot/share/logging/logAsyncWriter.cpp Show resolved Hide resolved

openjdk bot removed the ready Pull request is ready to be integrated label Sep 21, 2023

bridgekeeper bot closed this Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8292989: Avoid dynamic memory in AsyncLogWriter #1630

8292989: Avoid dynamic memory in AsyncLogWriter #1630

navyxliu commented Jul 28, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 28, 2023

openjdk bot commented Jul 28, 2023

mlbridge bot commented Jul 28, 2023 •

edited

Loading

openjdk bot commented Jul 28, 2023 •

edited

Loading

phohensee commented Jul 28, 2023

shipilev commented Aug 1, 2023

shipilev left a comment

shipilev Aug 1, 2023

navyxliu Aug 4, 2023

navyxliu Aug 4, 2023

shipilev Aug 1, 2023

navyxliu Aug 1, 2023

navyxliu commented Aug 1, 2023

tstuefe commented Aug 5, 2023

navyxliu commented Aug 7, 2023 •

edited

Loading

tstuefe commented Aug 7, 2023

shipilev commented Aug 7, 2023 •

edited

Loading

navyxliu commented Aug 9, 2023

tstuefe commented Aug 10, 2023

shipilev left a comment

navyxliu commented Aug 10, 2023

openjdk bot commented Sep 21, 2023

bridgekeeper bot commented Oct 19, 2023

bridgekeeper bot commented Nov 16, 2023

8292989: Avoid dynamic memory in AsyncLogWriter #1630

8292989: Avoid dynamic memory in AsyncLogWriter #1630

Conversation

navyxliu commented Jul 28, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 28, 2023

openjdk bot commented Jul 28, 2023

mlbridge bot commented Jul 28, 2023 • edited Loading

Webrevs

openjdk bot commented Jul 28, 2023 • edited Loading

phohensee commented Jul 28, 2023

shipilev commented Aug 1, 2023

shipilev left a comment

Choose a reason for hiding this comment

shipilev Aug 1, 2023

Choose a reason for hiding this comment

navyxliu Aug 4, 2023

Choose a reason for hiding this comment

navyxliu Aug 4, 2023

Choose a reason for hiding this comment

shipilev Aug 1, 2023

Choose a reason for hiding this comment

navyxliu Aug 1, 2023

Choose a reason for hiding this comment

navyxliu commented Aug 1, 2023

tstuefe commented Aug 5, 2023

navyxliu commented Aug 7, 2023 • edited Loading

tstuefe commented Aug 7, 2023

shipilev commented Aug 7, 2023 • edited Loading

navyxliu commented Aug 9, 2023

tstuefe commented Aug 10, 2023

shipilev left a comment

Choose a reason for hiding this comment

navyxliu commented Aug 10, 2023

openjdk bot commented Sep 21, 2023

bridgekeeper bot commented Oct 19, 2023

bridgekeeper bot commented Nov 16, 2023

navyxliu commented Jul 28, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 28, 2023 •

edited

Loading

openjdk bot commented Jul 28, 2023 •

edited

Loading

navyxliu commented Aug 7, 2023 •

edited

Loading

shipilev commented Aug 7, 2023 •

edited

Loading