Skip to content

AVRO-2346: Introduce JMH Performance Testing Framework#476

Closed
belugabehr wants to merge 2 commits into
apache:masterfrom
belugabehr:AVRO-2346
Closed

AVRO-2346: Introduce JMH Performance Testing Framework#476
belugabehr wants to merge 2 commits into
apache:masterfrom
belugabehr:AVRO-2346

Conversation

@belugabehr
Copy link
Copy Markdown
Contributor

No description provided.

@probot-autolabeler probot-autolabeler Bot added build Java Pull Requests for Java binding labels Mar 11, 2019
@belugabehr belugabehr changed the title First update AVRO-2346: Introduce JMH Performance Testing Framework Mar 11, 2019
@scottcarey
Copy link
Copy Markdown
Contributor

We'll want to test both in 'isolation' mode where each test is independent, and where all tests are warmed up before a single test is run (like https://hg.openjdk.java.net/code-tools/jmh/file/5984e353dca7/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_32_BulkWarmup.java).

There were massive differences in performance in the old Perf harness when commenting out tests because when one is run in isolation, the JVM can treat various method calls as monomorphic, but when multiple implementations of that method exist it has to switch to bimorphic or megamorphic dispatch.

@iemejia
Copy link
Copy Markdown
Member

iemejia commented Mar 26, 2019

We introduced automatic code formatting. For more info see the "How to contribute" page. This probably affected this PR can you please rebase it.

@belugabehr
Copy link
Copy Markdown
Contributor Author

@iemejia I changed code formatting as described and added capability to allow for bulk warmups as @scottcarey described

@belugabehr
Copy link
Copy Markdown
Contributor Author

@iemejia

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:1.20.0:check (spotless-check) on project avro-perf: Execution spotless-check of goal com.diffplug.spotless:spotless-maven-plugin:1.20.0:check failed: Unable to locate file with path: /testptch/unknown/lang/java/lang/java/eclipse-java-formatter.xml: Could not find resource '/testptch/unknown/lang/java/lang/java/eclipse-java-formatter.xml

Is this file missing?

@iemejia
Copy link
Copy Markdown
Member

iemejia commented Mar 27, 2019

@belugabehr The file is not missing, the issue is that it is resolved based on a property called main.basedir that you is not correctly defined in the new module this PR adds. Try adding <main.basedir>${project.parent.parent.basedir}</main.basedir> to the properties block in the pom file. It should work.

@belugabehr
Copy link
Copy Markdown
Contributor Author

@iemejia Thank you for the assistance. Tests are passing now. Please consider merging this request.

Copy link
Copy Markdown
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

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

LGTM, Merging manually to fix the maven module name.
I am not an expert on JMH but I think that having this in place for 1.9.0 will be perfect to start tackling regressions. Have you thought about a way to integrate this maybe with the CI @belugabehr ?

@belugabehr
Copy link
Copy Markdown
Contributor Author

Great question about regression testing. I'm not sure what the options are for such a thing.

I think for this first pass, it would be good to just allow folks to play with it and improve it.
As @scottcarey already started in on, performance testing requires a deep understanding of each test to understand if what we 'think' is being testing is actually being tested or is the compiler optimizing these micro-tests to the point where they are perhaps a single no-op. I wouldn't plan on making it part of any automated regression plan for a couple of release iterations.

@scottcarey
Copy link
Copy Markdown
Contributor

scottcarey commented Mar 28, 2019 via email

@iemejia iemejia closed this Mar 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Java Pull Requests for Java binding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants