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.

Sign up for GitHub

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

Jump to bottom

Avoid Optional.orElse(expression) #15301

Merged
merged 5 commits into from
Dec 6, 2022
Merged

Avoid Optional.orElse(expression) #15301

merged 5 commits into from
Dec 6, 2022

Conversation

findepi
Copy link
Member

@findepi findepi commented Dec 5, 2022

Evaluate option's alternative with orElseGet rather than eagerly with orElse(expression), when expression is expensive to evaluate. Here "expensive" means anything that can run a validation, i.e. is NOT constant NOR assessor, NOR a factory method of a simple type (like Optional, ImmutableList, and similar).

@findepi findepi added performance no-release-notes This pull request does not require release notes entry labels Dec 5, 2022
@cla-bot cla-bot bot added the cla-signed label Dec 5, 2022
@findepi findepi requested review from losipiuk and ebyhr December 5, 2022 21:22
@findepi findepi mentioned this pull request Dec 5, 2022
Merged
@github-actions github-actions bot added jdbc Relates to Trino JDBC driver tests:hive labels Dec 5, 2022
@findepi findepi requested a review from martint December 5, 2022 22:34
skrzypo987
skrzypo987 reviewed Dec 6, 2022
View reviewed changes
Copy link
Member

@skrzypo987 skrzypo987 left a comment

Choose a reason for hiding this comment

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

Majority of those cases should not be altered. The orElseGet should be used only when some actual work needs to be executed. Getters and trivial constructors are not expensive at all, creating lambdas is.

If any of places here are actual bottlenecks to the point that this is needed, we should get rid of Optional class first.

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/GatherAndMergeWindows.java Outdated Show resolved Hide resolved
@findepi
Copy link
Member Author

findepi commented Dec 6, 2022

Majority of those cases should not be altered.

Besides Result::empty, which ones?
Or, do you mean I should close the PR because it's all nonsense?

skrzypo987
skrzypo987 reviewed Dec 6, 2022
View reviewed changes
Copy link
Member

@skrzypo987 skrzypo987 left a comment

Choose a reason for hiding this comment

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

I've put (X) sign next to every change that IMHO should be rollbacked.
This is just my opinion, based on some experience, but opinion nonetheless.

The common misconception that you are following here is that object creation in Java is expensive. It is actually super cheap, especially if the object is short-lived.

Your approach is what I call a "defensive performance programming" which is by many considered the "root of all evil" just because they quote a book by Donald Knuth that they haven't even read. But everyone uses lazy string concatenation in logging statements which is exactly that.
I am actually a fan of this approach, but a bit more orthodox. I would just get rid of most of Optionals in the first place.

To sum up, the answer to the question "where to use eager and lazy orElse" is very simple - it depends.

client/trino-jdbc/src/main/java/io/trino/jdbc/TrinoDriverUri.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/Session.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/SetTimeZoneTask.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/metadata/PolymorphicScalarFunction.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/security/AccessControlManager.java Outdated Show resolved Hide resolved
...n/trino-hive/src/main/java/io/trino/plugin/hive/aws/athena/projection/IntegerProjection.java Show resolved Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/parquet/ParquetPageSourceFactory.java Outdated Show resolved Hide resolved
...trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControlMetadata.java Outdated Show resolved Hide resolved
plugin/trino-hudi/src/main/java/io/trino/plugin/hudi/HudiPageSourceProvider.java Outdated Show resolved Hide resolved
plugin/trino-tpch/src/main/java/io/trino/plugin/tpch/TpchMetadata.java Show resolved Hide resolved
@findepi
Avoid allocation in Result.empty()
e9c09f7 
The empty result constructor is often used in context of
`Optional.orElse(...)`, so it's better if it is cheap.
@findepi
Avoid allocation in Constraint.alwaysTrue and alwaysFalse
9d32ea3
@findepi findepi force-pushed the findepi/or-else branch 2 times, most recently from 9c66cb8 to 06dab22 Compare December 6, 2022 11:29
@findepi findepi requested a review from skrzypo987 December 6, 2022 11:30
@findepi
Copy link
Member Author

findepi commented Dec 6, 2022

Applied most comments. Thanks for your review @skrzypo987 !

@findepi
Avoid Optional.orElse(expression)
dd2b7f4 
Evaluate option's alternative with `orElseGet` rather than eagerly with
`orElse(expression)`, when expression is expensive to evaluate.
Here "expensive" means anything that can run a validation, i.e. is NOT
constant NOR assessor, NOR a factory method of a simple type (like
`Optional`, `ImmutableList`, and similar).
skrzypo987
skrzypo987 approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@skrzypo987 skrzypo987 left a comment

Choose a reason for hiding this comment

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

lgtm

plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraPartitionManager.java Show resolved Hide resolved
...n/trino-hive/src/main/java/io/trino/plugin/hive/aws/athena/projection/IntegerProjection.java Show resolved Hide resolved
@findepi findepi merged commit cebda34 into trinodb:master Dec 6, 2022
@findepi findepi deleted the findepi/or-else branch December 6, 2022 22:31
@github-actions github-actions bot added this to the 404 milestone Dec 6, 2022
@colebow colebow mentioned this pull request Dec 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skrzypo987 skrzypo987 skrzypo987 approved these changes

@losipiuk losipiuk losipiuk approved these changes

@raunaqmorarka raunaqmorarka Awaiting requested review from raunaqmorarka

@sopel39 sopel39 Awaiting requested review from sopel39

@ebyhr ebyhr Awaiting requested review from ebyhr

@martint martint Awaiting requested review from martint

Assignees
No one assigned
Labels
cla-signed jdbc Relates to Trino JDBC driver no-release-notes This pull request does not require release notes entry performance
Milestone
405
Development

Successfully merging this pull request may close these issues.
5 participants
@findepi @martint @losipiuk @raunaqmorarka @skrzypo987