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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ji] RubyString implements CharSequence #5180

Merged
merged 4 commits into from May 23, 2018

Conversation

Projects
None yet
2 participants
@kares
Member

kares commented May 21, 2018

found this 馃枌 at https://github.com/jruby/jruby/pull/3937/files#diff-2f8f0aefd8a8637512ce6c017da265b9

knew I saw a draft of this somewhere but did not find it until I checked some old PRs :)

... there's an opportunity here to have RubyString implements CharSequence
this is expected to fit nicely esp. will avoid conversions on logging libraries e.g. log4j2
(so one doesn't need to do an if check: logger.debug('foo') if logger.isDebugEnabled)

but its a breaking change in 2 ways :

  • RubyString#length needs to be changed (to return int) -> not expected to be used much
  • seeing a java.lang.CharSequence by JI will no longer conver to a java string

both seem acceptable for a major release such as 9.2 (we sure can not do this in a minor one)

@kares kares added this to the JRuby 9.2.0.0 milestone May 21, 2018

@kares kares requested review from headius and enebo May 21, 2018

@kares

This comment has been minimized.

Member

kares commented May 23, 2018

other way to avoid conversion (when seeing CharSequence type) would be to use ByteList
... which implements it as well but than it would need some shared marking upon the field leaving string.
and it isn't encoding to a Java string properly at this point (maybe with 2.0), right?

@headius

I have no issues with the actual code changes here.

I do worry about the length method changing signature but it does seem unlikely someone would use that rather than "strLength", and that's backward-compatible until everyone can be assured the "IRubyObject length()" is gone. We haven't really had a chance to verify this with any third-party extensions, though.

In general, I like this change at 9.2. I think we need to be more bold these days.

@kares kares merged commit 389eea3 into jruby:master May 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kares

This comment has been minimized.

Member

kares commented May 25, 2018

posting some micro-benchmarking numbers :

Benchmark                                        Mode  Cnt       Score      Error   Units
StringLoopBenchmark.benchLongDecodeStringLoop   thrpt    5   10745.123 卤  194.012  ops/ms
StringLoopBenchmark.benchLongJavaStringLoop     thrpt    5  123630.568 卤 2992.694  ops/ms
StringLoopBenchmark.benchLongRubyStringLoop     thrpt    5   20841.347 卤 1095.569  ops/ms
StringLoopBenchmark.benchShortDecodeStringLoop  thrpt    5   15207.295 卤  933.209  ops/ms
StringLoopBenchmark.benchShortJavaStringLoop    thrpt    5  140604.846 卤 2555.282  ops/ms
StringLoopBenchmark.benchShortRubyStringLoop    thrpt    5  103548.193 卤 3440.039  ops/ms

... instead of testing 'real-world' the case tries to do String#contentEquals(CharSequence)
decoding every time clearly has its price, on the other hand for short strings using the CharSequence interface without converting toString seems to be only about 40% worse than passing a native Java string.

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