Skip to content

Optimize: entrySet is faster than keySet + get to prevent N lookups#10496

Merged
wing328 merged 4 commits intoOpenAPITools:masterfrom
larrydiamond:master
Oct 4, 2021
Merged

Optimize: entrySet is faster than keySet + get to prevent N lookups#10496
wing328 merged 4 commits intoOpenAPITools:masterfrom
larrydiamond:master

Conversation

@larrydiamond
Copy link
Copy Markdown
Contributor

@larrydiamond larrydiamond commented Sep 29, 2021

This is a series of minor performance boost by replacing the keySet + get calls with entrySet in order to eliminate N lookups in a few places.

In addition, some TreeMap/Set objects were replaced with ConcurrentSkipListMap/Set types which are generally faster and intended to be a replacement when introduced with Java 1.6.
https://www.javacodegeeks.com/2017/04/simple-map-iterator-performance-test.html for some nice charts of the performance differences.

I ran all of the unit tests, and I was a little surprised to see pre-existing PMD issues. I did not add any new ones but there are existing ones.

I did review and follow the contribution guidelines and ran the samples. There were no file changes as a result of running the samples, which did not surprise me as this is a performance improvement only - there should be no changes in any output and there was not.

This contribution is against master, please let me know if I did this against the right branch or if I should be doing this work against a different branch.

This is my first pull request to openapi-generator, please excuse any expected actions I did not perform prior to sending this pull request, I will gladly make any changes to fulfill the process to commit code.

Thank you very much,
Larry Diamond

@wing328
Copy link
Copy Markdown
Member

wing328 commented Oct 2, 2021

Thanks for the PR.

cc @OpenAPITools/generator-core-team

@wing328 wing328 added this to the 5.3.0 milestone Oct 2, 2021
@wing328
Copy link
Copy Markdown
Member

wing328 commented Oct 2, 2021

@larrydiamond thanks for the PR but CI reports the following errors:

Error:  COMPILATION ERROR : 
Error:  /home/runner/work/openapi-generator/openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/ErlangClientCodegen.java:[329,43] incompatible types: java.lang.StringBuilder cannot be converted to java.lang.StringBuffer
Error:  /home/runner/work/openapi-generator/openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/ErlangClientCodegen.java:[332,32] incompatible types: java.lang.StringBuilder cannot be converted to java.lang.StringBuffer
Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project openapi-generator: Compilation failure: Compilation failure: 
Error:  /home/runner/work/openapi-generator/openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/ErlangClientCodegen.java:[329,43] incompatible types: java.lang.StringBuilder cannot be converted to java.lang.StringBuffer
Error:  /home/runner/work/openapi-generator/openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/ErlangClientCodegen.java:[332,32] incompatible types: java.lang.StringBuilder cannot be converted to java.lang.StringBuffer
Error:  -> [Help 1]
Error:  
Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.

Can you please take a look when you've time?

ref: https://github.com/OpenAPITools/openapi-generator/pull/10496/checks?check_run_id=3774266952

@larrydiamond
Copy link
Copy Markdown
Contributor Author

larrydiamond commented Oct 2, 2021

That's very odd, I will take a look. My apologies, maybe I missed a commit in there.

@larrydiamond
Copy link
Copy Markdown
Contributor Author

larrydiamond commented Oct 4, 2021

My apologies, I compiled using Java 11 which added the StringBuilder method that isnt there in Java 8. I'll remediate this right now with my apologies.

@larrydiamond
Copy link
Copy Markdown
Contributor Author

larrydiamond commented Oct 4, 2021

The unit tests have run to completion on my mac using Java 8 now. I checked the three other calls to appendReplacement and two other calls to appendTail and all of the calls are using StringBuffer.

@larrydiamond
Copy link
Copy Markdown
Contributor Author

larrydiamond commented Oct 4, 2021

And then the bitrise build failed due to something that looks like it just needs to be retried
[ERROR] Failed to execute goal on project openapi-generator: Could not resolve dependencies for project org.openapitools:openapi-generator:jar:5.3.0-SNAPSHOT: Could not transfer artifact org.jetbrains.kotlin:kotlin-compiler-embeddable:jar:1.3.60 from/to central (https://repo.maven.apache.org/maven2): GET request of: org/jetbrains/kotlin/kotlin-compiler-embeddable/1.3.60/kotlin-compiler-embeddable-1.3.60.jar from central failed: Connection reset -> [Help 1]

@wing328
Copy link
Copy Markdown
Member

wing328 commented Oct 4, 2021

No need to apologize. We appreciate your contributions to his project.

Bitrise CI failure is not related to this change so don't worry about it.

@wing328 wing328 merged commit 10b310d into OpenAPITools:master Oct 4, 2021
@larrydiamond
Copy link
Copy Markdown
Contributor Author

larrydiamond commented Oct 4, 2021

I'm very happy to help out here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimschubert jimschubert Awaiting requested review from jimschubert

Assignees

No one assigned

Labels

Enhancement: Feature Feature: Generator

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

2 participants

@larrydiamond @wing328