TEZ-4275: Use Google Guava Intern Facility by belugabehr · Pull Request #95 · apache/tez

belugabehr · 2021-01-28T21:47:41Z

No description provided.

belugabehr · 2021-02-08T20:07:56Z

+
+ // Hint to the JVM to clean up anything no longer needed; be ready for the next DAG
+ System.gc();
+ System.runFinalization();
+


I'm not sure if it is helpful to role this change into this ticket or not. However, the clearCache stuff is pretty useless. All it does is make the map entries available for garbage collection,... which they already are by design.
However, this here will hint to the JDK to actually clean up that stuff. Hinting to the JDK at this points seems like a reasonable thing to do between DAGs, but I'm not sure if it's worthwhile.

belugabehr · 2021-02-10T18:52:53Z

OK @jteagles @abstractdog this PR is failing on some pre-existing find bugs error. Kind of funny actually:

Null passed for non-null parameter of com.google.common.base.Strings.isNullOrEmpty(String)

The method has isNull in the name of it, of course it can accept a null value. :D

Please consider these changes as they should lower memory requirements, provide faster concurrent access, and simplify the code a bit. In particular, please consider my thoughts here:

#95 (comment)

abstractdog · 2021-02-10T19:29:02Z

OK @jteagles @abstractdog this PR is failing on some pre-existing find bugs error. Kind of funny actually:
Null passed for non-null parameter of com.google.common.base.Strings.isNullOrEmpty(String)
The method has isNull in the name of it, of course it can accept a null value. :D
Please consider these changes as they should lower memory requirements, provide faster concurrent access, and simplify the code a bit. In particular, please consider my thoughts here:
#95 (comment)

yeah, absolutely, we've also found similar isNull* madness in https://issues.apache.org/jira/browse/TEZ-4215, I believe we should merge that @jteagles, and @belugabehr you can handle this in a similar way...if findbugs warning simply doesn't makes sense, let's try to ignore it

belugabehr · 2021-03-02T18:29:56Z

@abstractdog @jteagles Any thoughts on this?

jteagles · 2021-03-02T20:10:06Z

@belugabehr can you compare speed and memory consumption with current implementation? I have a test that can be used to measure.
https://issues.apache.org/jira/browse/TEZ-1526
https://issues.apache.org/jira/secure/attachment/12857710/TEZ-1526.memory.test.patch
mvn clean test -Dtest=TestTezIds#testIdGetInstancePerformance -pl tez-common -am

Then compare

tez-common/target/surefire-reports/org.apache.tez.dag.records.TestTezIds.txt
tez-common/target/surefire-reports/org.apache.tez.dag.records.TestTezIds-output.txt

belugabehr · 2021-03-03T20:58:50Z

== TEZ-4275 ==
TEZ-1526: heap used memory: 100000:26299128
TEZ-1526: heap used memory: 200000:12867000
TEZ-1526: heap used memory: 300000:46693400
TEZ-1526: heap used memory: 400000:76540520
TEZ-1526: heap used memory: 500000:106386920
TEZ-1526: heap used memory: 600000:40765776
TEZ-1526: heap used memory: 700000:70625496
...
TEZ-1526: heap used memory: 9300000:115818312
TEZ-1526: heap used memory: 9400000:143028440
TEZ-1526: heap used memory: 9500000:177041136
TEZ-1526: heap used memory: 9600000:204251344
TEZ-1526: heap used memory: 9700000:231461552
TEZ-1526: heap used memory: 9800000:265474312
TEZ-1526: heap used memory: 9900000:292684504
TEZ-1526: heap used memory: 10000000:326697216

== master ==
TEZ-1526: heap used memory: 100000:29985368
TEZ-1526: heap used memory: 200000:20945736
TEZ-1526: heap used memory: 300000:54775896
TEZ-1526: heap used memory: 400000:84626224
TEZ-1526: heap used memory: 500000:15007832
TEZ-1526: heap used memory: 600000:48851936
TEZ-1526: heap used memory: 700000:78714376
...
TEZ-1526: heap used memory: 9300000:171467736
TEZ-1526: heap used memory: 9400000:198175528
TEZ-1526: heap used memory: 9500000:224883320
TEZ-1526: heap used memory: 9600000:258268056
TEZ-1526: heap used memory: 9700000:284975880
TEZ-1526: heap used memory: 9800000:318360616
TEZ-1526: heap used memory: 9900000:345068408
TEZ-1526: heap used memory: 10000000:44603528

== TEZ-4275 ==
-------------------------------------------------------------------------------
Test set: org.apache.tez.dag.records.TestTezIds
-------------------------------------------------------------------------------
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 7.648 s - in org.apache.tez.dag.records.TestTezIds
== master ==
-------------------------------------------------------------------------------
Test set: org.apache.tez.dag.records.TestTezIds
-------------------------------------------------------------------------------
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 7.937 s - in org.apache.tez.dag.records.TestTezIds

belugabehr · 2021-03-03T21:00:43Z

Results match my expectations. If nothing else, Guava uses a Map with the cached value (as key) and a dummy singleton value. Current implementation uses a Map with the cached value (as key) and a new value. That'll save some memory.

belugabehr · 2021-03-03T23:01:05Z

Also, I suspect Guava version is better in multi-threaded environment since it locks on "regions" of the map instead of locking on the entire map (master). I ran a few quick and dirty sanity checks by wrapping the proposed unit test in a thread and launching 4 of them...

master: 3.7s - 4.2s
TEZ-4275: 3.4s - 3.6s

belugabehr · 2021-03-03T23:04:11Z

16 Threads yielded a similar 10% improvement in throughput

belugabehr · 2021-03-11T16:08:45Z

[2021-02-10T18:17:40.551Z] Reason | Tests
[2021-02-10T18:17:40.551Z] FindBugs | module:tez-dag [2021-02-10T18:17:40.551Z] | org.apache.tez.dag.app.DAGAppMaster.handle(DAGAppMasterEvent) forces garbage collection; extremely dubious except in benchmarking code At DAGAppMaster.java:dubious except in benchmarking code At DAGAppMaster.java:[line 872]

Hello, what do you think about this PR as it currently stands?

jteagles · 2021-03-11T16:12:34Z

If the GC isn't strictly needed can it be taken out?

belugabehr · 2021-03-11T16:15:42Z

@jteagles Yes. It can be.

There may be some value though to hinting that now (between DAGs) is a good time to cleanup, and I was mimicking the previous code that was attempting to cleanup these caches. Just let me know, I don't have a feeling (or data) on it one way or another.

belugabehr · 2021-03-20T17:11:04Z

@jteagles What would your preference be here?

belugabehr · 2021-05-06T15:12:54Z

@jteagles @abstractdog OK. I took out the GC hint and it builds without the warning.

Thanks!

hadoop-yetus · 2021-05-06T15:21:01Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	17m 55s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
+0 🆗	mvndep	4m 25s	Maven dependency ordering for branch
+1 💚	mvninstall	9m 10s	master passed
+1 💚	compile	3m 20s	master passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	compile	2m 59s	master passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	checkstyle	2m 51s	master passed
+1 💚	javadoc	2m 56s	master passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	2m 40s	master passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+0 🆗	spotbugs	0m 43s	Used deprecated FindBugs config; considering switching to SpotBugs.
+0 🆗	findbugs	0m 41s	tez-tools/analyzers/job-analyzer in master has 4 extant findbugs warnings.
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 9s	Maven dependency ordering for patch
+1 💚	mvninstall	2m 9s	the patch passed
+1 💚	compile	1m 57s	the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javac	1m 57s	the patch passed
+1 💚	compile	1m 38s	the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	javac	1m 38s	the patch passed
+1 💚	checkstyle	0m 9s	tez-common: The patch generated 0 new + 25 unchanged - 1 fixed = 25 total (was 26)
+1 💚	checkstyle	0m 9s	The patch passed checkstyle in tez-runtime-internals
+1 💚	checkstyle	0m 14s	tez-runtime-library: The patch generated 0 new + 30 unchanged - 1 fixed = 30 total (was 31)
+1 💚	checkstyle	0m 25s	The patch passed checkstyle in tez-dag
+1 💚	checkstyle	0m 9s	The patch passed checkstyle in tez-history-parser
+1 💚	checkstyle	0m 9s	tez-tools/analyzers/job-analyzer: The patch generated 0 new + 18 unchanged - 2 fixed = 18 total (was 20)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	1m 32s	the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	1m 23s	the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	findbugs	4m 32s	the patch passed
		_ Other Tests _
+1 💚	unit	0m 29s	tez-common in the patch passed.
+1 💚	unit	0m 31s	tez-runtime-internals in the patch passed.
+1 💚	unit	4m 56s	tez-runtime-library in the patch passed.
+1 💚	unit	4m 8s	tez-dag in the patch passed.
+1 💚	unit	2m 11s	tez-history-parser in the patch passed.
+1 💚	unit	2m 24s	job-analyzer in the patch passed.
+1 💚	asflicense	0m 50s	The patch does not generate ASF License warnings.
		83m 58s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-95/6/artifact/out/Dockerfile
GITHUB PR	#95
JIRA Issue	TEZ-4275
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname	Linux dcbb343a39ac 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/tez.sh
git revision	master / `0184781`
Default Java	Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-95/6/testReport/
Max. process+thread count	789 (vs. ulimit of 5500)
modules	C: tez-common tez-runtime-internals tez-runtime-library tez-dag tez-plugins/tez-history-parser tez-tools/analyzers/job-analyzer U: .
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-95/6/console
versions	git=2.25.1 maven=3.6.3 findbugs=3.0.1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

belugabehr · 2021-06-10T03:19:46Z

@abstractdog Just a gentle reminder about this PR.

@pgaref Do you know anyone that can assist here?

abstractdog · 2021-06-10T06:06:41Z

hi @belugabehr! I just ran through the history, memory/speed improvement is promising, also you removed every questioned part, so this looks good to me
it's a +1 from my side

belugabehr · 2021-06-30T13:42:30Z

@abstractdog Can you please assist with a merge? Thanks!

jteagles · 2021-06-30T14:47:43Z

+1. Got distracted away from this PR. I'll do the merge shortly.

belugabehr · 2021-06-30T14:55:06Z

No worries! Thanks @jteagles

* TEZ-4275: Use Google Guava Intern Facility * Also add hint for running finalization * Use Guava String Intern implementation * Fix checkstyle, white-space issues * Remove GC hint Co-authored-by: David Mollitor <david.mollitor@cloudera.com> (cherry picked from commit 984d09c)

belugabehr added 2 commits January 28, 2021 16:46

TEZ-4275: Use Google Guava Intern Facility
2d69511

Also add hint for running finalization
020fe0c

belugabehr closed this Feb 8, 2021

belugabehr reopened this Feb 8, 2021

belugabehr commented Feb 8, 2021
View reviewed changes

belugabehr added 2 commits February 8, 2021 16:49

Use Guava String Intern implementation
07d8554

Fix checkstyle, white-space issues
c088da6

Remove GC hint
93487e9

jteagles merged commit 984d09c into apache:masterJun 30, 2021

Uh oh!

Conversation

belugabehr commented Jan 28, 2021

Uh oh!

belugabehrFeb 8, 2021

Choose a reason for hiding this comment

Uh oh!

belugabehr commented Feb 10, 2021

Uh oh!

abstractdog commented Feb 10, 2021

Uh oh!

belugabehr commented Mar 2, 2021

Uh oh!

jteagles commented Mar 2, 2021

Uh oh!

belugabehr commented Mar 3, 2021

Uh oh!

belugabehr commented Mar 3, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

belugabehr commented Mar 3, 2021

Uh oh!

belugabehr commented Mar 3, 2021

Uh oh!

belugabehr commented Mar 11, 2021

Uh oh!

jteagles commented Mar 11, 2021

Uh oh!

belugabehr commented Mar 11, 2021

Uh oh!

belugabehr commented Mar 20, 2021

Uh oh!

belugabehr commented May 6, 2021

Uh oh!

hadoop-yetus commented May 6, 2021

Uh oh!

belugabehr commented Jun 10, 2021

Uh oh!

abstractdog commented Jun 10, 2021

Uh oh!

belugabehr commented Jun 30, 2021

Uh oh!

jteagles commented Jun 30, 2021

Uh oh!

belugabehr commented Jun 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

belugabehr commented Mar 3, 2021•
edited
Loading