Skip to content

Refactor conditional statements to switch expression in Dictionary class - #16313

Merged
dweiss merged 4 commits into
apache:mainfrom
rajat315315:sing_switch
Jul 21, 2026
Merged

Refactor conditional statements to switch expression in Dictionary class#16313
dweiss merged 4 commits into
apache:mainfrom
rajat315315:sing_switch

Conversation

@rajat315315

@rajat315315 rajat315315 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #16314

@github-actions github-actions Bot added this to the 11.0.0 milestone Jul 1, 2026
@rajat315315

Copy link
Copy Markdown
Contributor Author

@gsmiller @msokolov
This is a simple code refactoring Pull request.

@msokolov

Copy link
Copy Markdown
Contributor

Why would we do this? We don't want to be making changes just for the sake of making changes. It looks as if you must be running some kind of bot that is generating automated code change PRs. For me, that is unwelcome. I don't have time to review endless trivial bot-generated PRs - also why are you calling me out by name? This isn't code I have any particular association with. It's generally not a good idea to invoke individuals without a good reason. I hope you'll take this criticism in a constructive spirit and devote your energy to meaningful contributions to this project - thanks!

@rajat315315

Copy link
Copy Markdown
Contributor Author

If we see in the code, there's already a TODO defined for improvement. I thought why not refactor and help.
// TODO: convert to a switch? at Line no. 359

@dweiss dweiss added the skip-changelog Apply to PRs that don't need a changelog entry, stopping the automated changelog check. label Jul 21, 2026
@dweiss

dweiss commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

There is actually some value in this conversion since switch statements on strings should compile to faster code than a series of ifs - I'll merge this.

@dweiss

dweiss commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Please run gradlew tidy and clean up formatting next time, prior to filing a PR.

@rajat315315

rajat315315 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

There is actually some value in this conversion since switch statements on strings should compile to faster code than a series of ifs

Yes, correct. There are 44 if-else statements in this code fragment..
But a switch-case should do it in 1-2 instructions only.
Thanks.

@dweiss
dweiss merged commit 224f580 into apache:main Jul 21, 2026
13 checks passed
@dweiss

dweiss commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

It's compiled into a table lookup over the input string's hashCode. How it's optimized within hotspot - I've no idea.
https://docs.oracle.com/en/java/javase/26/docs/specs/jvms/jvms-3.html#jvms-3.10

@msfroh

msfroh commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

It's compiled into a table lookup over the input string's hashCode. How it's optimized within hotspot - I've no idea. https://docs.oracle.com/en/java/javase/26/docs/specs/jvms/jvms-3.html#jvms-3.10

Ooh! I know this one.

Initially it'll do a binary search over the valid hashCodes (lookupswitch), then do the String#equals check to rule out a hash collision. Based on that, it will istore the ordinal of the reached branch to decide which code to jump to (tableswitch). In general, we're talking O(log n) versus the if/else chain's O(n). That's the code that will show up in the javap output, before any Hotspot optimization.

Hotspot can make a couple of possible optimizations to the lookupswitch: unroll the binary search, and potentially move hot cases before the binary search. I think that second one can be a big deal. Because String#equals is a method call, AFAIK the C2 compiler can't move hot cases to the top of an if-else chain, because it doesn't know for sure that each if body is side-effect-free, whereas any switch condition is guaranteed to be.

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

Labels

module:analysis skip-changelog Apply to PRs that don't need a changelog entry, stopping the automated changelog check.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimizing too many if-else into a switch-case

4 participants