You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/05/09 04:40:23 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
LuciferYang opened a new pull request, #36484:
URL: https://github.com/apache/spark/pull/36484
### What changes were proposed in this pull request?
There is lock contention between `LevelDBIterator.finalize()` and `LevelDB.close()` method. If `LevelDB.close()` method is executed first, the `LevelDBIterator`s hold by `Finalizer` will not close before the `LevelDB` close.
This pr try to use a custom `ReferenceQueue + cleanupThread` instead of `Finalization` mechanism to automatically clean up `LevelDBIterator` resources that are not explicitly closed in Spark code.
### Why are the changes needed?
Fix potential risk `LevelDBIterator` resource leaks caused by lock contention.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
- Pass GA and add a new test to check all LevelDBIterator not explicit closed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36484:
URL: https://github.com/apache/spark/pull/36484#issuecomment-1123128398
Friendly ping @dongjoon-hyun both https://github.com/apache/spark/pull/36484 and https://github.com/apache/spark/pull/36467 are ready for review. Could you help to review them? Thanks ~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #36484:
URL: https://github.com/apache/spark/pull/36484#issuecomment-1168158936
Actually, I rechecked this PR during last weekend already before you pinged me Today, but I couldn't make me sure about the proposed approach. Please note that I didn't make any objections here. It's simply not sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36484:
URL: https://github.com/apache/spark/pull/36484#issuecomment-1123249323
> GC difference after this PR?
Do you mean JVM GC?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36484:
URL: https://github.com/apache/spark/pull/36484#issuecomment-1123327781
change `COUNT` to 102400 and add `-XX:+PrintGC -XX:+PrintGCDetails -XX:+PrintGCTimeStamps` to observe GC behavior:
**Before**
```
OpenJDK 64-Bit Server VM (25.312-b07) for linux-amd64 JRE (Zulu 8.58.0.13-CA-linux64) (1.8.0_312-b07), built on Oct 9 2021 03:04:03 by "zulu_re" with gcc 4.4.7 20120313 (Red Hat 4.4.7-23)
Memory: 4k page, physical 527627268k(39637496k free), swap 0k(0k free)
CommandLine flags: -XX:+IgnoreUnrecognizedVMOptions -XX:InitialHeapSize=2147483648 -XX:MaxHeapSize=4294967296 -XX:MaxMetaspaceSize=2147483648 -XX:+PrintGC -XX:+PrintGCDetails -XX:+PrintGCTimeStamps -XX:ReservedCodeCacheSize=134217728 -XX:ThreadStackSize=4096 -XX:+UseCompressedClassPointers -XX:+UseCompressedOops -XX:+UseParallelGC
4.229: [GC (Allocation Failure) [PSYoungGen: 524800K->25093K(611840K)] 524800K->25109K(2010112K), 0.1434697 secs] [Times: user=5.47 sys=0.23, real=0.15 secs]
6.878: [GC (Allocation Failure) [PSYoungGen: 549893K->30226K(1136640K)] 549909K->30250K(2534912K), 0.1407982 secs] [Times: user=4.57 sys=0.03, real=0.14 secs]
10.916: [GC (Allocation Failure) [PSYoungGen: 1079826K->56841K(1136640K)] 1079850K->56873K(2534912K), 0.5514541 secs] [Times: user=22.90 sys=0.54, real=0.55 secs]
14.788: [GC (Allocation Failure) [PSYoungGen: 1106441K->39200K(1310720K)] 1106473K->39240K(2708992K), 0.4692478 secs] [Times: user=22.50 sys=0.28, real=0.47 secs]
20.524: [GC (Allocation Failure) [PSYoungGen: 1262880K->51227K(1275392K)] 1262920K->51275K(2673664K), 0.7022824 secs] [Times: user=28.76 sys=0.38, real=0.70 secs]
25.734: [GC (Allocation Failure) [PSYoungGen: 1274907K->56794K(1279488K)] 1274955K->56850K(2677760K), 0.3269271 secs] [Times: user=10.53 sys=0.05, real=0.33 secs]
31.718: [GC (Allocation Failure) [PSYoungGen: 1274842K->56160K(1285120K)] 1274898K->58980K(2683392K), 0.3651417 secs] [Times: user=12.49 sys=0.11, real=0.37 secs]
Heap
PSYoungGen total 1285120K, used 518037K [0x000000076ab00000, 0x00000007c0000000, 0x00000007c0000000)
eden space 1218048K, 37% used [0x000000076ab00000,0x0000000786e0d198,0x00000007b5080000)
from space 67072K, 83% used [0x00000007b5080000,0x00000007b8758368,0x00000007b9200000)
to space 70144K, 0% used [0x00000007bbb80000,0x00000007bbb80000,0x00000007c0000000)
ParOldGen total 1398272K, used 2819K [0x00000006c0000000, 0x0000000715580000, 0x000000076ab00000)
object space 1398272K, 0% used [0x00000006c0000000,0x00000006c02c0fc0,0x0000000715580000)
Metaspace used 19352K, capacity 19716K, committed 19840K, reserved 1067008K
class space used 2325K, capacity 2422K, committed 2432K, reserved 1048576K
```
**After**
```
OpenJDK 64-Bit Server VM (25.312-b07) for linux-amd64 JRE (Zulu 8.58.0.13-CA-linux64) (1.8.0_312-b07), built on Oct 9 2021 03:04:03 by "zulu_re" with gcc 4.4.7 20120313 (Red Hat 4.4.7-23)
Memory: 4k page, physical 527627268k(39516972k free), swap 0k(0k free)
CommandLine flags: -XX:+IgnoreUnrecognizedVMOptions -XX:InitialHeapSize=2147483648 -XX:MaxHeapSize=4294967296 -XX:MaxMetaspaceSize=2147483648 -XX:+PrintGC -XX:+PrintGCDetails -XX:+PrintGCTimeStamps -XX:ReservedCodeCacheSize=134217728 -XX:ThreadStackSize=4096 -XX:+UseCompressedClassPointers -XX:+UseCompressedOops -XX:+UseParallelGC
4.080: [GC (Allocation Failure) [PSYoungGen: 524800K->24653K(611840K)] 524800K->24669K(2010112K), 0.2088060 secs] [Times: user=8.93 sys=0.14, real=0.21 secs]
6.767: [GC (Allocation Failure) [PSYoungGen: 549453K->29881K(1136640K)] 549469K->29905K(2534912K), 0.1403327 secs] [Times: user=4.42 sys=0.02, real=0.14 secs]
10.577: [GC (Allocation Failure) [PSYoungGen: 1079481K->55196K(1136640K)] 1079505K->55228K(2534912K), 0.3690663 secs] [Times: user=12.57 sys=0.04, real=0.37 secs]
14.225: [GC (Allocation Failure) [PSYoungGen: 1104796K->40043K(1310720K)] 1104828K->40083K(2708992K), 0.4300225 secs] [Times: user=20.10 sys=0.25, real=0.43 secs]
20.011: [GC (Allocation Failure) [PSYoungGen: 1263723K->51450K(1275392K)] 1263763K->51498K(2673664K), 0.3348333 secs] [Times: user=11.46 sys=0.09, real=0.34 secs]
25.481: [GC (Allocation Failure) [PSYoungGen: 1275130K->56619K(1278976K)] 1275178K->56675K(2677248K), 0.4491326 secs] [Times: user=16.92 sys=0.12, real=0.45 secs]
31.391: [GC (Allocation Failure) [PSYoungGen: 1274155K->56192K(1285120K)] 1274211K->58965K(2683392K), 0.4748197 secs] [Times: user=19.71 sys=0.14, real=0.48 secs]
Heap
PSYoungGen total 1285120K, used 505049K [0x000000076ab00000, 0x00000007c0000000, 0x00000007c0000000)
eden space 1217536K, 36% used [0x000000076ab00000,0x00000007861563e8,0x00000007b5000000)
from space 67584K, 83% used [0x00000007b5000000,0x00000007b86e0368,0x00000007b9200000)
to space 70656K, 0% used [0x00000007bbb00000,0x00000007bbb00000,0x00000007c0000000)
ParOldGen total 1398272K, used 2772K [0x00000006c0000000, 0x0000000715580000, 0x000000076ab00000)
object space 1398272K, 0% used [0x00000006c0000000,0x00000006c02b53c0,0x0000000715580000)
Metaspace used 19373K, capacity 19786K, committed 19840K, reserved 1067008K
class space used 2337K, capacity 2423K, committed 2432K, reserved 1048576K
```
Before `2.71s (0.15 + 0.14 + 0.55 + 0.47 + 0.70 + 0.33 + 0.37)` , after `2.42s (0.21 + 0.14 + 0.37 + 0.43 + 0.34 + 0.45 + 0.48)`, no significant change in GC behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #36484:
URL: https://github.com/apache/spark/pull/36484#issuecomment-1123252371
Yes~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36484:
URL: https://github.com/apache/spark/pull/36484#issuecomment-1168187685
In fact, the `cleaningThread` added by current pr in this pr is similar to `o.a.s.ContextCleaner#cleaningThread` and `o.a.s.storage.BlockManager.RemoteBlockDownloadFileManager#cleaningThread`.
The difference is that the latter two allow `cleaningThread.interrupt()` but the `cleaningThread` added by current pr not allowed due to I want to to ensure the elements in `referenceQueue` are closed before `cleaningThread` stopped.
I think `AutoCloseable`+ `java Lang.ref.Cleaner` maybe a better solution, `try-with-resources` has been used as much as possible in the current Spark code to ensure that `DBIterator` is closed, but `Cleaner` is only supported after Java 9, So we can also wait until `Cleaner` is available before continuing to sort out this part of the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36484:
URL: https://github.com/apache/spark/pull/36484#discussion_r867629141
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java:
##########
@@ -282,4 +273,24 @@ static int compare(byte[] a, byte[] b) {
return a.length - b.length;
}
+ static class LevelDBIteratorWeakReference extends WeakReference<LevelDBIterator<?>> {
+
+ private final DBIterator it;
+
+ LevelDBIteratorWeakReference(
+ LevelDBIterator<?> referent,
+ ReferenceQueue<? super LevelDBIterator<?>> q) {
+ super(referent, q);
+ it = referent.it;
+ }
+
+ public void close() {
+ try{
Review Comment:
Unlike `RocksDBIterator`, `DBIterator.close()` may throw IOE. Since it is not be handled to when using the `Finalization` mechanism, it will not be handled here too . Or change to `com.google.common.io.Closeables.close(it, swallowIOException= false)` to log the exception?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36484:
URL: https://github.com/apache/spark/pull/36484#issuecomment-1123287877
@dongjoon-hyun Update LevelDBBenchmark to pr description, the performance of `dbclose` is significantly worse because after this pr because `dbclose` have to wait until `cleaningThread` exits (wait for `referenceQueue.remove(1000L)`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36484:
URL: https://github.com/apache/spark/pull/36484#issuecomment-1121402089
Now I just infer that similar issue also exist in leveldb because it is very similar to rocksdb， but I'm really not sure because I haven't been able to manually compile leveldb for debugging at present
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang closed pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang closed pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
URL: https://github.com/apache/spark/pull/36484
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36484:
URL: https://github.com/apache/spark/pull/36484#issuecomment-1168152436
friendly ping @dongjoon-hyun , will we not accept this pr due to the performance degradation of close? If yes, I will close this pr and https://github.com/apache/spark/pull/36467 , because I haven't found a way to solve the problem without losing `close` performance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36484:
URL: https://github.com/apache/spark/pull/36484#discussion_r867629141
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java:
##########
@@ -282,4 +273,24 @@ static int compare(byte[] a, byte[] b) {
return a.length - b.length;
}
+ static class LevelDBIteratorWeakReference extends WeakReference<LevelDBIterator<?>> {
+
+ private final DBIterator it;
+
+ LevelDBIteratorWeakReference(
+ LevelDBIterator<?> referent,
+ ReferenceQueue<? super LevelDBIterator<?>> q) {
+ super(referent, q);
+ it = referent.it;
+ }
+
+ public void close() {
+ try{
Review Comment:
Unlike `RocksDBIterator`, `DBIterator.close()` may throw IOE. Since it is not be handled to when using the `Finalization` mechanism, it will not be handled here too . Or change to `com.google.common.io.Closeables.close(it, swallowIOException= false)` to log the IOE?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36484:
URL: https://github.com/apache/spark/pull/36484#discussion_r867629141
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java:
##########
@@ -282,4 +273,24 @@ static int compare(byte[] a, byte[] b) {
return a.length - b.length;
}
+ static class LevelDBIteratorWeakReference extends WeakReference<LevelDBIterator<?>> {
+
+ private final DBIterator it;
+
+ LevelDBIteratorWeakReference(
+ LevelDBIterator<?> referent,
+ ReferenceQueue<? super LevelDBIterator<?>> q) {
+ super(referent, q);
+ it = referent.it;
+ }
+
+ public void close() {
+ try{
Review Comment:
Unlike `RocksDBIterator`, `LevelDBIterator` may throw IOE. Since it is not be handled to when using the `Finalization` mechanism, it will not be handled here too . Or change to `com.google.common.io.Closeables.close(it, swallowIOException= false)` to log the exception?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36484: [SPARK-39127][CORE] Fix potential risk of `LevelDBIterator` leak
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36484:
URL: https://github.com/apache/spark/pull/36484#discussion_r867627116
##########
common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDB.java:
##########
@@ -385,6 +380,33 @@ private byte[] getTypeAlias(Class<?> klass) throws Exception {
return alias;
}
+ private void stopCleanupThread() throws InterruptedException {
+ if (cleaningThread != null) {
+ stopped = true;
+ cleaningThread.join();
+ }
+ }
+
+ private void keepCleaning() {
+ boolean lastIsPresent = false;
+ while (!stopped || lastIsPresent) {
+ try {
+ Optional<? extends Reference<? extends LevelDBIterator<?>>> removed =
+ Optional.ofNullable(referenceQueue.remove(1000L));
+ lastIsPresent = removed.isPresent();
+ System.out.println("lastIsPresent is " + lastIsPresent);
Review Comment:
will delete this line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org