Skip to content

Conversation

@dfa1
Copy link
Contributor

@dfa1 dfa1 commented Dec 5, 2021

@andimarek @bbakerman hello :)

I would like to propose the following optimization in FieldLevelTrackingApproach. By using a custom data structure (a glorified int[] array), it is possible to gain some performance.

I wrote 2 performance tests:

  • JMH IntMapBenchmark (included in the PR) on my machine, I get 5x better performance:
Benchmark                                Mode  Cnt        Score         Error  Units
IntMapBenchmark.benchmarkIntMap         thrpt   10  5698421.790 ± 1355795.218  ops/s
IntMapBenchmark.benchmarkLinkedHashMap  thrpt   10  1089616.718 ±   72838.075  ops/s
  • quickperf memory test (not included in the PR):
@RunWith(QuickPerfJUnitRunner.class)
public class QuickPerfTest {

	@Test
	@MeasureHeapAllocation
	public void measureLinkedHashMap() {
		for (int j = 0; j < 1000; j++) {
			Map<Integer, Integer> result = new LinkedHashMap<>();
			for (int i = 0; i < 30; i++) {
				int level = i % 10;
				int count = i * 2;
				result.put(level, result.getOrDefault(level, 0) + count);
				assert result.get(level) != -1;
			}
		}
	}

	@Test
	@MeasureHeapAllocation
	public void measureIntMap() {
		for (int j = 0; j < 1000; j++) {
			IntMap result = new IntMap(16);
			for (int i = 0; i < 30; i++) {
				int level = i % 10;
				int count = i * 2;
				result.increase(level, count);
				assert result.get(level) != -1;
			}
		}
	}
}

Output:

IntMap
[QUICK PERF] Measured heap allocation (test method thread): 100.21 Kilo bytes (102 616 bytes)

LinkedHashMap
[QUICK PERF] Measured heap allocation (test method thread): 524.09 Kilo bytes (536 664 bytes)

please have a look and let me know... I really don't like the current name as it is not a real replacement for a Map<Integer, Integer>! :-)

@dfa1 dfa1 force-pushed the perf-field-level-tracking branch 2 times, most recently from a388455 to 1b25d41 Compare December 5, 2021 20:10
By replacing LinkedHashMap<Integer, Integer> with a simpler
int[] it is possible to observe about -80% memory consumption
(avoiding a lot of primitive int boxing, better cache locality, etc).
@dfa1 dfa1 force-pushed the perf-field-level-tracking branch from 1b25d41 to 7806ff2 Compare December 5, 2021 20:20
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for this PR. I am happy to accept it in the general sense but if we can tweak the naming and also have the set method that would be unreal


CallStack() {
expectedStrategyCallsPerLevel.put(1, 1);
expectedStrategyCallsPerLevel.increment(1, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should make another method that is a "set". The above code is "set level 1 to the value 1" whereas your code is "increment" the value by 1 (but its zero right now)

So I think a expectedStrategyCallsPerLevel.set(1) is a better method

@bbakerman bbakerman added this to the 18.0 milestone Dec 6, 2021
@bbakerman
Copy link
Member

Actually since this is so small - let me fix it up

@bbakerman bbakerman merged commit f8dd9d2 into graphql-java:master Dec 6, 2021
@dfa1 dfa1 deleted the perf-field-level-tracking branch December 6, 2021 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants