[test] HeapSizeEstimator #1281

FelixGV · 2024-11-04T22:25:00Z

Introduced a new HeapSizeEstimator utility to make our on-heap memory usage assessment more accurate, and easier to maintain as class hierarchies evolve.

For now it is only used in a unit test, not yet in the main code.

How was this PR tested?

New unit tests.

Does this PR introduce any user-facing changes?

No. You can skip the rest of this section.
Yes. Make sure to explain your proposed changes and call out the behavior change.

gaojieliu · 2024-11-05T17:56:07Z

Have you got a chance to take a look at openjdk/jol implementation?
https://github.com/openjdk/jol
It might carry some issues, but I guess we can analyze the strategy this lib is using to see whether we can borrow some or not.

FelixGV · 2024-11-05T20:53:42Z

internal/venice-common/src/main/java/com/linkedin/venice/memory/HeapSizeEstimator.java

+/**
+ * Utility class to help in implementing {@link Measurable#getSize()}. A couple of important points:
+ *
+ * 1. This utility class does not "measure" the heap size, but rather attempts to "predict" it, based on knowledge of
+ *    the internals of the JVM. If any of the assumptions are wrong, then of course the results will be inaccurate.
+ * 2. This utility class assumes we are using the HotSpot JVM.
+ */
+public class HeapSizeEstimator {


Starting a thread to respond to @gaojieliu's general comment:

Have you got a chance to take a look at openjdk/jol implementation?
https://github.com/openjdk/jol
It might carry some issues, but I guess we can analyze the strategy this lib is using to see whether we can borrow some or not.

I posted this work on social media and received a lot of interesting examples from folks about other open source projects with the same or similar purpose. I'll maintain a list of these here:

https://github.com/openjdk/jol

https://github.com/ehcache/sizeof

https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/util/RamUsageEstimator.java

For now, this work looks pretty similar to (at least some parts of) the above, but I think there is still potential to have a good implementation as part of Venice. In particular, I want a solution in which there is no hot path reflection. For now, this tradeoff is not apparent because I have not yet included examples of how we can leverage this utility in the rest of our code, but I will add it soon when the basic functionality is stabilized...

The reflection mentioned here is mainly about performance, right?
Have you tried these alternatives and run it with JMH?
I tried jol and the initialization is slow, but after that, it is fairly fast.

I did a small test against jol and it seems it can work properly with Oracle OpenJDK8 release, but not MSFT OpenJDK11 release.

@alex-dubrouski
Do you know why MSFT-jdk11 won't work well with attached agent to measure the object size?
I tried to add this jvm arg, but it still doesn't work:

-Djdk.attach.allowAttachSelf=true

And both jol and sizeof are suffering from the same issue with JDK11 as they are using the similar technologies..

I will check a bit later

It turned out that msft-jdk11.0.8/11.0.13 has a bug, which triggered the crash while using 'jol' or 'sizeof' lib and JDK17 apparently fixed this issue.

Well, Felix, that's why we should probably use JOL :D ;)

But where is the learning value in that, @alex-dubrouski :D ?

Besides, this memory estimation code needs to run in DVC, which is one of our client libs, meaning we cannot tightly control which JVM it's going to run in. Ideally, it should not depend on agents or finicky Unsafe code...

That being said, using JOL in unit test to validate the accuracy of this utility might be a good idea 🤔

BTW... you did not answer my question @alex-dubrouski 😅 but I found out that the hashIsZero was added 5 years ago: openjdk/jdk@89a267c

According to JDK-8221836, this change got released in Java 13, which is weird because I thought I was looking at the Java 17 source code in my IDE, but apparently not; it was probably hanging on to the source of an older Java version.

And indeed, I can get it to show me the proper source and see that it's there now.

alex-dubrouski · 2024-11-06T17:15:44Z

internal/venice-common/src/main/java/com/linkedin/venice/memory/HeapSizeEstimator.java

+
+  /** Deal with alignment by rounding up to the nearest alignment boundary. */
+  private static int roundUpToNearestAlignment(int size) {
+    int partialAlignmentWindowUsage = size % ALIGNMENT_SIZE;


Alignment could be modified with command line.

I know that it can be configured at start time. But are you saying it can be modified at run time?

No, it can't be changed at runtime, but you defined static alignment which might not be accurate. That's an edge case of course, I was just saying.

Oh I see... you mean that alignment can be overridden by a JVM option, in which case it wouldn't decide just based on whether it's a 32 bits or 64 bits VM?

Is it via -XX:ObjectAlignmentInBytes?

Yes, it is set statically at bootstrap, but code does not take this into account. Feel free to ignore that, since this is very rare edge case.

alex-dubrouski · 2024-11-06T17:16:42Z

internal/venice-common/src/main/java/com/linkedin/venice/memory/HeapSizeEstimator.java

+     * {@link Class#getDeclaredFields()} returns the fields (of all visibility, from private to public and everything
+     * in between) of the class, but not of its parent class.
+     */
+    for (Field f: c.getDeclaredFields()) {


Some of the fields are not visible. For example there are native references.

Could you point me to a class (perhaps from the JDK) that has a native reference in it? I'd like to test that scenario and see what happens...

Java.lang.Object :)

Then I'm definitely not taking it into account 😅 ... where can I learn more about this?

Alex Shipilev explained that in one of the videos (video is not in English...)

internal/venice-common/src/main/java/com/linkedin/venice/memory/HeapSizeEstimator.java

Introduced a new HeapSizeEstimator utility to make our on-heap memory usage assessment more accurate, and easier to maintain as class hierarchies evolve. For now it is only used in a unit test, not yet in the main code.

… measures the allocated memory.

gaojieliu · 2024-11-07T17:52:00Z

internal/venice-common/src/main/java/com/linkedin/venice/memory/HeapSizeEstimator.java

+   * @throws {@link StackOverflowError} It should be noted that this function is recursive in nature and so any class
+   *         which contains a loop in its class graph will cause a stack overflow.
+   */
+  public static <T> int getClassOverhead(Class<T> c) {


So this function is mainly calculating the class overhead, not the deep size, right?
If the object contains a nested hashmap, this class will only measure the overhead of the class, and the user needs to manually add the class overhead on top of the hashmap cost, which needs to be calculated manually?

I think it will be useful to have a function to measure both class overhead and the actual data.

The above requirement is mainly about measuring the deep size of the object.

If we do want to implement such feature, it will be great if we can have some filtering logic to ignore certain types of fields, which can be referring to some common references.

Yes, this function does not measure the deep size of an object, and that is also the reason why it takes a Class param and not an Object. Essentially, my idea is to hand-code the "variable" part of the measurement of the classes of interest, and the function in this utility is just to make it easier to have the "base cost" (the non-variable part). This utility is not intended to be called in the hot path.

I will integrate the usage of this utility into the MemoryBoundBlockingQueue measurement code. I have not done so yet because I want to be extra sure the utility is as accurate as possible (which it may or may not be, as I am not sure if I have chased all of the bugs properly yet, but at least it should be pretty close...).

FelixGV commented Nov 5, 2024

View reviewed changes

FelixGV force-pushed the memory_usage_estimation branch from 94ff779 to 626a47d Compare November 5, 2024 20:54

alex-dubrouski reviewed Nov 6, 2024

View reviewed changes

internal/venice-common/src/main/java/com/linkedin/venice/memory/HeapSizeEstimator.java Outdated Show resolved Hide resolved

FelixGV force-pushed the memory_usage_estimation branch from c3fda54 to 16ae593 Compare November 6, 2024 21:29

FelixGV added 4 commits November 7, 2024 08:11

[test] HeapSizeEstimator

3a3ff9b

Introduced a new HeapSizeEstimator utility to make our on-heap memory usage assessment more accurate, and easier to maintain as class hierarchies evolve. For now it is only used in a unit test, not yet in the main code.

Fixed many bugs discovered as part of a new test which experimentally…

525e9bc

… measures the allocated memory.

Fixed bug related to "superclass gaps", and improved testing.

fd43cfe

Fixed static analysis and made measurement a little more lenient.

c39e456

FelixGV force-pushed the memory_usage_estimation branch from 16ae593 to c39e456 Compare November 7, 2024 16:49

Added memoization, as suggest by Alex D., and refactored a little bit.

9f1ef31

gaojieliu reviewed Nov 7, 2024

View reviewed changes

FelixGV added 2 commits November 7, 2024 11:44

Static analysis appeasement.

2533615

More tests and minor cleanups...

0ee7e0b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test] HeapSizeEstimator #1281

[test] HeapSizeEstimator #1281

FelixGV commented Nov 4, 2024

gaojieliu commented Nov 5, 2024

FelixGV Nov 5, 2024

gaojieliu Nov 6, 2024

gaojieliu Nov 7, 2024

gaojieliu Nov 7, 2024

alex-dubrouski Nov 7, 2024

gaojieliu Nov 8, 2024

alex-dubrouski Nov 8, 2024

FelixGV Nov 8, 2024

FelixGV Nov 8, 2024

FelixGV Nov 8, 2024

alex-dubrouski Nov 6, 2024

FelixGV Nov 7, 2024

alex-dubrouski Nov 7, 2024

FelixGV Nov 7, 2024

alex-dubrouski Nov 7, 2024

alex-dubrouski Nov 6, 2024

FelixGV Nov 7, 2024

alex-dubrouski Nov 7, 2024

FelixGV Nov 7, 2024

alex-dubrouski Nov 7, 2024

gaojieliu Nov 7, 2024

gaojieliu Nov 7, 2024

FelixGV Nov 7, 2024

[test] HeapSizeEstimator #1281

Are you sure you want to change the base?

[test] HeapSizeEstimator #1281

Conversation

FelixGV commented Nov 4, 2024

How was this PR tested?

Does this PR introduce any user-facing changes?

gaojieliu commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment