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

Optimize helidon optionals - io.helidon.common.mapper.OptionalValue #505

Merged
merged 4 commits into from
Oct 9, 2024

Conversation

re-thc
Copy link
Contributor

@re-thc re-thc commented Oct 8, 2024

Helidon docs says that Optional is expensive and don't use it unless you need to. In the generator, optionals are more of an implementation detail and null or the default value is returned. For better performance, this PR replaces optionals with contains / get as Helidon suggests.

re-thc and others added 2 commits October 8, 2024 21:45
Helidon docs says that `Optional` is expensive and don't use it unless you need to.
In the generator, optionals are more of an implementation detail and null or the default value is returned.
For better performance, this PR replaces optionals with contains / get as Helidon suggests.
@SentryMan
Copy link
Collaborator

Sure that makes sense, but it would seem the tests are failing. Try running your mvn commands with -Ptest. In addition, the helidon jsonb test module has a test that you can debug to set breakpoints during generation

@re-thc
Copy link
Contributor Author

re-thc commented Oct 8, 2024

Thanks for the tip. Updated the code and ran the test locally with the fix.

@rob-bygrave
Copy link
Contributor

Helidon docs says that Optional is expensive and don't use it unless you need to.

Well yes, it's actually an io.helidon.common.mapper.OptionalValue and not a java.util.Optional and yes, looking at the source it does look ugly in there (especially given we are only interested in the first String value).

It seems like an API miss to have the most common case sub-optimal with the 2 lookups of the key ... hmmm.

This is a nice improvement though, thanks !!

@rob-bygrave rob-bygrave changed the title Optimize helidon optionals Optimize helidon optionals - io.helidon.common.mapper.OptionalValue Oct 9, 2024
@rob-bygrave rob-bygrave added this to the 2.8 milestone Oct 9, 2024
@rob-bygrave rob-bygrave merged commit 1c27559 into avaje:master Oct 9, 2024
6 checks passed
@re-thc
Copy link
Contributor Author

re-thc commented Oct 9, 2024

@rob-bygrave thanks and yes in regards to the Helidon source there's a lot of great high level ideas, but it's definitely not as well optimized / polished like Netty or Jetty. There's likely some 20-40% performance to be gained just by tweaking some functions / flows. There's also missing validation and spec compliance so expect slowdowns (there's already a PR to fix host validation that looks heavy).

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.

3 participants