HDDS-9528. Managed objects should not override finalize() #5853

duongkame · 2023-12-21T20:56:06Z

What changes were proposed in this pull request?

Simple and efficient (to be verified) way to do leak detection without relying on finalize().

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9528

How was this patch tested?

CI.

duongkame · 2023-12-22T03:26:16Z

I think this can be much simpler to implement within RocksDB java lib (and a good contribution to RocksDB too). I will submit a patch to rocksdb, but we need to proceed with this change too.

adoroszlai

Thanks @duongkame for the patch, LGTM.

Few trivial items noted. I'll commit them to the PR soon to trigger CI.

I also have a few ideas for minor improvements to be done as a follow-up. Filed the task for myself for now.

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/LeakDetector.java

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/package-info.java

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/LeakTracker.java

szetszwo

@duongkame , thanks a lot for working on this! Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13065566/5853_review.patch

BTW, have you done some benchmarks to see if this can improve the performance?

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/LeakDetector.java

szetszwo · 2023-12-22T18:02:29Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/LeakDetector.java

+ * limitations under the License.
+ *
+ */
+package org.apache.hadoop.hdds.resource;


It should use the org.apache.hadoop.hdds.utils package.

@duongkame , any comments on this?

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/LeakTracker.java

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/LeakDetector.java

szetszwo · 2023-12-22T18:08:23Z

...d-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksObjectUtils.java

-  static void assertClosed(RocksObject rocksObject, String stackTrace) {
+  private static final LeakDetector LEAK_DETECTOR = new LeakDetector("ManagedRocksObject");
+
+  static LeakTracker track(AutoCloseable object, @Nullable StackTraceElement[] stackTrace) {


Use RocksObject instead of AutoCloseable.

Get stackTrace inside this method instead of passing it in order to reduce code duplication.

Get stackTrace inside this method instead of passing it in order to reduce code duplication.

This one I also included in HDDS-10000. Since the stackTrace members already exist, i.e. the duplication is not introduced here, I thought it can be done as a follow-up.

There's also RocksMutableObject and other custom object in the same package to be tracked here. AutoCloseable is the most convenient here. Btw, it's package private and only used for managed object in this package.

Use RocksObject instead of AutoCloseable.
Get stackTrace inside this method instead of passing it in order to reduce code duplication.

This is great. Thanks.

Increment startIndex (to 4) here:

ozone/hadoop-hdds/managed-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksObjectUtils.java

Lines 73 to 74 in 333e6a1

static String formatStackTrace(@Nullable StackTraceElement[] elements) {

return HddsUtils.formatStackTrace(elements, 3);

szetszwo · 2023-12-22T18:08:49Z

...d-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksObjectUtils.java

-    if (rocksObject.isOwningHandle()) {
-      reportLeak(rocksObject, stackTrace);
-    }
+    final String name = object.getClass().getSimpleName();


Use object.toString() instead.

@duongkame , any comments on this?

I don't think we need anything specific in toString(). Plus, toString gives an uncertain performance.

I fine to use Class.getSimpleName() although it is known to have bad performance; see https://stackoverflow.com/questions/17369304/why-is-class-getsimplename-not-cached
It seems minor.

...d-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksObjectUtils.java

szetszwo · 2023-12-22T18:28:37Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/LeakDetector.java

+ *
+ * }</pre>
+ */
+public class LeakDetector implements Runnable {


Use generic type and don't implements Runnable.

szetszwo · 2023-12-22T18:32:24Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/LeakDetector.java

+    Thread t = new Thread(this);
+    t.setName(LeakDetector.class.getSimpleName() + "-" + name);
+    t.setDaemon(true);
+    LOG.info("Starting leak detector thread {}.", name);
+    t.start();
+  }
+
+  @Override
+  public void run() {


Make a private and pass it using new Thread(this::run).

Make sense.

szetszwo

... have you done some benchmarks to see if this can improve the performance?

@duongkame , have you got a chance to run some benchmarks?

szetszwo · 2023-12-28T16:29:36Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/LeakDetector.java

+ * limitations under the License.
+ *
+ */
+package org.apache.hadoop.hdds.resource;


@duongkame , any comments on this?

szetszwo · 2023-12-28T16:30:20Z

...d-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksObjectUtils.java

-    if (rocksObject.isOwningHandle()) {
-      reportLeak(rocksObject, stackTrace);
-    }
+    final String name = object.getClass().getSimpleName();


@duongkame , any comments on this?

duongkame · 2024-01-03T02:10:26Z

... have you done some benchmarks to see if this can improve the performance?

@duongkame , have you got a chance to run some benchmarks?

I didn't run benchmarks on any ozone scenarios. However, I did a simple benchmark scenario each creating 100M managed objects with a percentage of leaks. The new solution consistently beats the old one by a 33% gap.

New solution result:

Finish benchmarking. Total time (ms): 38008
Total objects: 100001000
Total leaks found: 1000

Old solution (with finalizer) result.

Finish benchmarking. Total time (ms): 56527
Total objects: 100001000
Total leaks found: 1000

Benchmarking code:

public class ManagedObjectBenchmarking {
  public static void main(String[] args) throws Exception {
    NativeLibraryLoader.getInstance().loadLibrary("/tmp");
    long start = System.currentTimeMillis();

    int rounds = 100;
    for (int i = 0; i < rounds; i++) {
      create(1_000_000, true);
      create(10, false);
      System.gc();
    }

    long duration = System.currentTimeMillis() - start;
    Thread.sleep(1000);

    System.out.println("Finish benchmarking. Total time (ms): " + duration);
    System.out.println("Total objects: " + ManagedRocksObjectMetrics.INSTANCE.totalManagedObjects());
    System.out.println("Total leaks found: " + ManagedRocksObjectMetrics.INSTANCE.totalLeakObjects());
  }
  private static void create(int n, boolean close) {
    for (int i = 0; i < n; i++) {
      ManagedWriteBatch object = new ManagedWriteBatch();
      if (close) {
        object.close();
      }
    }
  }
}

Theoretically, this improvement unblocks GC and optimizes leak detection by only processing unclosed resources (closed ones are not enqueued).

szetszwo

+1 the change looks good. Just some very minor comments.

szetszwo · 2024-01-03T03:29:04Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/resource/package-info.java

+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Contains utilities for resource management.
+ */
+package org.apache.hadoop.hdds.resource;


This file should be removed.

szetszwo · 2024-01-03T03:29:55Z

...d-rocksdb/src/main/java/org/apache/hadoop/hdds/utils/db/managed/ManagedRocksObjectUtils.java

-    if (rocksObject.isOwningHandle()) {
-      reportLeak(rocksObject, stackTrace);
-    }
+    final String name = object.getClass().getSimpleName();


I fine to use Class.getSimpleName() although it is known to have bad performance; see https://stackoverflow.com/questions/17369304/why-is-class-getsimplename-not-cached
It seems minor.

szetszwo · 2024-01-03T03:33:03Z

... The new solution consistently beats the old one by a 33% gap.

Good to know, thanks!

(Sorry that I thought you would run the same OM performance benchmarks. I understand that ref queue is better solution than finalizer. However, I am not sure if it would help the OM performance.)

adoroszlai · 2024-01-03T18:38:04Z

Thanks @duongkame for the patch, @szetszwo for the review.

(cherry picked from commit b13f01c)

duongkame added 5 commits December 20, 2023 10:33

HDDS-9528. Managed objects should not override finalize()

78994d9

Fix test.

a5bace9

Correct approach.

ba3c85b

Testing leak detection.

6e6ca6c

Cleanup.

3045642

duongkame requested review from adoroszlai and szetszwo December 21, 2023 21:13

duongkame marked this pull request as ready for review December 21, 2023 21:13

duongkame added 6 commits December 21, 2023 13:15

Cleanup.

65477d5

Checksylte.

e4c2500

Use GC from CodecTestUtil.

96f1e2d

Unit test for LeakDetector.

d87f173

checkstyle.

ceaf506

Correct docs.

0eb4031

adoroszlai reviewed Dec 22, 2023

View reviewed changes

Fix typo and dangling javadoc

79b0ea6

szetszwo reviewed Dec 22, 2023

View reviewed changes

duongkame and others added 3 commits December 22, 2023 17:46

Reduce code duplications.

333e6a1

Update startIndex.

69bf9d5

Merge remote-tracking branch 'origin/master' into HDDS-9528

d082bee

adoroszlai requested a review from szetszwo December 28, 2023 10:00

szetszwo reviewed Dec 28, 2023

View reviewed changes

duongkame added 2 commits January 2, 2024 18:47

Move LeakDetector to utils package.

8e19927

Move LeakDetector to utils package.

7c9ddf8

szetszwo approved these changes Jan 3, 2024

View reviewed changes

duongkame added 2 commits January 2, 2024 22:26

Avoid calling getSimpleName for each ManagedObject creation.

263fb65

Remove package-info.

63304d0

Merge remote-tracking branch 'origin/master' into HDDS-9528

5a98898

adoroszlai merged commit b13f01c into apache:master Jan 3, 2024

adoroszlai pushed a commit to adoroszlai/ozone that referenced this pull request Jan 25, 2024

HDDS-9528. Managed objects should not override finalize() (apache#5853)

5b6ccee

(cherry picked from commit b13f01c)

adoroszlai mentioned this pull request Jan 25, 2024

[DO NOT MERGE] Backport some fixes from master to ozone-1.4 #6096

Merged

duongkame deleted the HDDS-9528 branch April 12, 2025 00:12

	static String formatStackTrace(@Nullable StackTraceElement[] elements) {
	return HddsUtils.formatStackTrace(elements, 3);

HDDS-9528. Managed objects should not override finalize() #5853

HDDS-9528. Managed objects should not override finalize() #5853

Uh oh!

Conversation

duongkame commented Dec 21, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

duongkame commented Dec 22, 2023

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szetszwo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duongkame Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duongkame commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Jan 3, 2024

Uh oh!

adoroszlai commented Jan 3, 2024

Uh oh!

Reviewers

szetszwo left a comment •

edited

Loading

duongkame Dec 23, 2023 •

edited

Loading

duongkame commented Jan 3, 2024 •

edited

Loading