HBASE-27445 fix the result of DirectMemoryUtils#getDirectMemorySize #4846
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Oct 26, 2022
| @@ -85,7 +85,8 @@ public static long getDirectMemorySize() { | |||
| RuntimeMXBean runtimemxBean = ManagementFactory.getRuntimeMXBean(); | |||
| List<String> arguments = runtimemxBean.getInputArguments(); | |||
| long multiplier = 1; // for the byte case. | |||
| for (String s : arguments) { | |||
| for (int i = arguments.size() - 1; i >= 0; i--) { | |||
There was a problem hiding this comment.
Does this method guarantee the order of the returned arguments?
There was a problem hiding this comment.
Thanks for reviewing Duo.
From what I see, the result is in order.
https://github.com/Tencent/TencentKona-8/blob/master/hotspot/src/share/vm/services/management.cpp#L545
|
💔 -1 overall
This message was automatically generated. |
|
I mean if there is a standard on this? The code of a specific jvm implementation is not stable, maybe other implementations will have different orders... If not, let's at least check the order for at least several commonly used jvm implementations, and also add comments in code to say that this is just our observation, not a standard. |
Really sorry for missing your reply here Duo. I didn't find related information in the jvm specification in https://docs.oracle.com/javase/specs/jvms/se8/html/ , so I guess there isn't ? I'll do the check and add some comments. Thanks for your reply Duo. |
|
Hi Duo I found that netty did some work about this. I did a simple test, it works for jdk8, jdk11 and jdk17. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Hi, Duo, mind taking a look ? @Apache9 |
+1 since we have shaded netty. |
Apache9
reviewed
Nov 16, 2022
| return retValue * multiplier; | ||
| } | ||
| try { | ||
| Field directMemoryLimit = PlatformDependent.class.getDeclaredField("MAX_DIRECT_MEMORY"); |
There was a problem hiding this comment.
PlatformDependent.maxDirectMemory is enough?
There was a problem hiding this comment.
Thanks for reviewing Duo @Apache9
I think maybe we cannot. Here is the reason:
-
in the master branch, the hbase-thirdparty version is 4.1.2
https://github.com/apache/hbase/blob/master/pom.xml#L870 -
the the hbase-thirdparty 4.1.2, the netty version is 4.1.82.Final
https://github.com/apache/hbase-thirdparty/blob/230e5ebc432c2004b4ab744dca6627697f9f2a76/pom.xml#L135 -
in the netty 4.1.82.Final, the return value is DIRECT_MEMORY_LIMIT
https://github.com/netty/netty/blob/47799635143d7a11b56c4a4e9a1e65ca221d28ca/common/src/main/java/io/netty/util/internal/PlatformDependent.java#L407
however, the value can be overrided if org.apache.hbase.thirdparty.io.netty.maxDirectMemory is set.
https://github.com/netty/netty/blob/47799635143d7a11b56c4a4e9a1e65ca221d28ca/common/src/main/java/io/netty/util/internal/PlatformDependent.java#L179
So I think this API is not stable enough. That's the reason why I give up this.
There was a problem hiding this comment.
OK, on branch 4.1 it returns the netty limit. Should we provide a fallback way if netty one is avaiable?
There was a problem hiding this comment.
In both version , the meaning of MAX_DIRECT_MEMORY is the same, It represents the direct memory size that the JVM process can obtain. So I think the value of MAX_DIRECT_MEMORY is fine. Maybe there's no need to fallback ?
There was a problem hiding this comment.
We use reflection to get this field so it will not trigger a compilation error if netty changes this field in the future, so we can not find it when upgrading netty.
The estimateMaxDirectMemory method is public, so maybe we could just call it and store the return value by our own?
There was a problem hiding this comment.
Oh, in the old version, this method is private. Didn't notice it has been public now. Thanks for point it out.
You are right，this is better than reflection. I'll fix this. Thanks Duo.@Apache9
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Nov 17, 2022
| * @return the setting of -XX:MaxDirectMemorySize as a long. Returns 0 if -XX:MaxDirectMemorySize | ||
| * is not set. | ||
| */ | ||
| /** Returns the direct memory limit of the current progress */ | ||
| public static long getDirectMemorySize() { |
There was a problem hiding this comment.
Will this method be called by multiple threads? Let's initialize it while loading the class?
There was a problem hiding this comment.
Currently not likely, so I make this lazily.
But I think you are right, it's better to evaluate it first. Pushed a new commit to address this. Thanks Duo. @Apache9
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Nov 21, 2022
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…pache#4846) Co-authored-by: huiruan <huiruan@tencent.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 2f4758e) (cherry picked from commit a3ac3c5) Change-Id: Ic0711a7076e3bfad7fb35d40f33aaf82b22c77a2
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…rySize (apache#4846)" This reverts commit bf84f6a. Change-Id: Ice546e5c31cb937cf9410f9c466ae2d7ad383810
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…pache#4846) Co-authored-by: huiruan <huiruan@tencent.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 2f4758e) (cherry picked from commit a3ac3c5) Change-Id: Ibe0d43401f3745d4ae6d399620ec55002f5effe7
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.