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

[ji] RubyString implements CharSequence #5180

Merged
merged 4 commits into from May 23, 2018
Merged

[ji] RubyString implements CharSequence #5180

merged 4 commits into from May 23, 2018

Conversation

kares
Copy link
Member

@kares 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 08:47
@kares
Copy link
Member Author

kares commented May 23, 2018 •
edited

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
headius reviewed May 23, 2018
View reviewed changes
Copy link
Member

@headius headius left a comment

Choose a reason for hiding this comment

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

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
@kares
Copy link
Member Author

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.

@georgie84 georgie84 mentioned this pull request Nov 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@headius headius headius left review comments

@enebo enebo Awaiting requested review from enebo

Assignees
No one assigned
Labels
java integration
Projects
None yet
Milestone
JRuby 9.2.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kares @headius