Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting up JMH for PartiQL #427

Merged
merged 6 commits into from
Sep 28, 2021
Merged

Setting up JMH for PartiQL #427

merged 6 commits into from
Sep 28, 2021

Conversation

abhikuhikar
Copy link
Contributor

Description of changes:
Sets up JMH for PartiQL

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Comment on lines 63 to 64
implementation 'org.openjdk.jmh:jmh-core:0.9'
implementation 'org.openjdk.jmh:jmh-generator-annprocess:0.9'
Copy link
Member

@dlurton dlurton Sep 13, 2021

Choose a reason for hiding this comment

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

You're adding a dependency on JMH to the maven package here. We don't want that. I think the reason this is required is because the benchmarks reside under the src directory, which should not be.

@@ -0,0 +1,163 @@
package org.partiql.lang
Copy link
Member

@dlurton dlurton Sep 13, 2021

Choose a reason for hiding this comment

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

This must not reside in this directory since that will be adding it to the .jar we publish to maven and the location of this file is also why you had to add the implementation dependencies on the JMH packages in order to get it to build.

This file should really reside in lang/jmh/org/partiql/lang instead of here. To make this work, you need to modify the build.gradle in the root project by adding the lines noted below:

    plugins.withId('java', { _ ->
        sourceSets {
            main.java.srcDirs = ["src"]
            main.resources.srcDirs = ["resources"]
            test.java.srcDirs = ["test"]
            test.resources.srcDirs = ["test-resources"]

            // add these lines
            jmh.java.srcDirs = ["jmh"]
            jmh.resources.srcDirs = ["jmh-resources"]
        }        
    })

This may not be 100% exactly right but I'm certain something like this will be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created the jmh sub-project but for some reason the benchmarks need to be under ./..src/jmh/kotlin/.. folder structure. I tried some other directory structures but the benchmark list wasn't populated for any other directory structure.

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2021

Codecov Report

Merging #427 (a16b1de) into main (bb0ddea) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #427      +/-   ##
============================================
+ Coverage     82.29%   82.31%   +0.02%     
  Complexity     1394     1394              
============================================
  Files           171      171              
  Lines         10725    10716       -9     
  Branches       1769     1766       -3     
============================================
- Hits           8826     8821       -5     
+ Misses         1358     1355       -3     
+ Partials        541      540       -1     
Flag Coverage Δ
CLI 18.18% <ø> (ø)
EXAMPLES 74.85% <ø> (ø)
LANG 84.81% <ø> (+0.02%) ⬆️
PTS ∅ <ø> (∅)
TEST_SCRIPT 79.68% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ng/src/org/partiql/lang/util/IonValueExtensions.kt 44.06% <0.00%> (-0.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb0ddea...a16b1de. Read the comment docs.

abhikuhikar and others added 2 commits September 22, 2021 08:29
Also remove jmh sub-project and configure the jmh source-set of the
`lang` project to point to `lang/jmh`.
dlurton
dlurton previously approved these changes Sep 23, 2021
Copy link
Member

@dlurton dlurton left a comment

Choose a reason for hiding this comment

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

See the commit I added. No need for a separate project, just need to change the default location for the jmh source set that's added by the JMH plugin.

@abhikuhikar abhikuhikar merged commit 5b3769f into main Sep 28, 2021
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.

4 participants