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

JIT fixnum cases (with a value span <= 32) as a tableswitch #4430

Merged
merged 1 commit into from Apr 19, 2017

Conversation

@kares
Copy link
Member

kares commented Jan 9, 2017

... instead of falling back to lookupswitch

noticed (while fixing #4429) that only "perfect" consecutive fixnum cases generate a tableswitch

javac does some table/lookup space cost calc to decide, here just a simple size bound (of 32) is implemented

case 0, 10, 20, 30 will generate a table-switch (filling in 'wholes' e.g. case 1: default)
while case 0, 10, 50 will generate a lookup-switch just like before

@kares kares added this to the JRuby 9.1.7.0 milestone Jan 9, 2017
@kares
Copy link
Member Author

kares commented Jan 9, 2017

... feel free to postpone to next release if you feel like it could use some more work (e.g. a different max)

@enebo
Copy link
Member

enebo commented Jan 11, 2017

@kares It seems reasonable to me but I wonder how often this happens to warrant the extra code. I guess JVM does it so they thought it was worth it.

Second nit is you are still not putting your closing } onto same line :)

@kares kares modified the milestones: JRuby 9.1.8.0, JRuby 9.1.7.0 Jan 12, 2017
... instead of falling back to lookupswitch
@kares kares force-pushed the test-jit-tableswitch branch from 3a0e1ee to 3d080ce Jan 12, 2017
@kares
Copy link
Member Author

kares commented Jan 12, 2017

yep, sorry about the style changes (although in this case I might have done it on purpose - the if () gets lost in dense lines bellow and above but I shall respect your style taste :).

since an update some time back my IDEA has never been the same - was glad to get the jruby project working again I need to spent some time setting it up as I need to manually undo changes it does for me.

will do a micro bench test whether its worth the change (should be since its expected to jump directly skiping the lookup part) -> will post back with numbers.
... such casing should be not that uncommon e.g. for def my(*args); methods

@kares kares modified the milestones: JRuby 9.2.0.0, JRuby 9.1.8.0 Mar 6, 2017
@olleolleolle
Copy link
Member

olleolleolle commented Apr 12, 2017

Super-interesting deep-dive into a detail.

(Hi @kares, could you choose another name than p for a local variable in the Ruby tests?

My eyes – and some code highlighters – read that as Kernel#p.)

@kares
Copy link
Member Author

kares commented Apr 16, 2017

thanks @olleolleolle ... sorry not into style updates really at this point (this code might get rejected).

have done some micro benchmarks previously (as @enebo suggested) and the change seemed only make a little difference, so I am not sure if tableswitch makes sense over lookupswitch if it 'complicates' code. but than maybe I have done it wrong - someone better understanding the difference in terms of JVM (and native code jitting) should decide whether this makes sense or not really ...

@headius
Copy link
Member

headius commented Apr 19, 2017

This looks fine. Merging so it can bake for 9.1.9.0.

@headius headius merged commit 27b38db into master Apr 19, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@kares kares deleted the test-jit-tableswitch branch Apr 19, 2017
@kares kares modified the milestones: JRuby 9.1.9.0, JRuby 9.2.0.0 Apr 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.