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

Fix tidy_bytes for JRuby #13919

Merged
merged 1 commit into from
Feb 10, 2014
Merged

Conversation

jcoyne
Copy link
Contributor

@jcoyne jcoyne commented Feb 2, 2014

The previous implementation was broken because JRuby (1.7.10) doesn't
have a code converter for UTF-8 to UTF8-MAC.

@jcoyne
Copy link
Contributor Author

jcoyne commented Feb 2, 2014

@norman @burke can I get your input on this?

@jcoyne
Copy link
Contributor Author

jcoyne commented Feb 2, 2014

@headius I wouldn't mind your review either.

@burke
Copy link
Contributor

burke commented Feb 3, 2014

I wouldn't be surprised if this turns out to be measurably slower. I chose UTF-8 Mac because it's so nearly identical that I figured it should be pretty trivial to transcode, whereas UTF-16 is a completely different encoding.

I don't have any numbers to back this hunch up, however, and even if it is slower, it may not make much of a difference in real cases.

It should, in theory, work. I can't think of any case where a character would be representable in UTF-8 but not UTF-16 (or vice versa).

Comment at the top of the method is in need of minor rewording too.

@jcoyne
Copy link
Contributor Author

jcoyne commented Feb 3, 2014

I don't think it should be any slower. It will have a larger memory footprint, but seems better to get this working on JRuby than to save a few bytes.

@tenderlove
Copy link
Member

Should we merge this?

@norman
Copy link
Contributor

norman commented Feb 9, 2014

Has anyone actually benchmarked it? We've all chipped away at this method
over the years to make it fast, I'd hate to take a step backwards. In the
event it's slower on MRI we could just set the transitional encoding
conditionally based on the Ruby platform. I would do it myself right now
but I'm traveling.

Sent from my phone

@jcoyne
Copy link
Contributor Author

jcoyne commented Feb 9, 2014

Here's a benchmark run on ruby 2.1.0p0. I don't show any appreciable difference.

https://gist.github.com/jcoyne/8905114

@norman
Copy link
Contributor

norman commented Feb 9, 2014

Awesome. Let's merge it!

@arunagw
Copy link
Member

arunagw commented Feb 9, 2014

This will require a rebase with master as well

The previous implementation was broken because JRuby (1.7.10) doesn't
have a code converter for UTF-8 to UTF8-MAC.
@jcoyne
Copy link
Contributor Author

jcoyne commented Feb 10, 2014

@arunagw rebased.

@burke
Copy link
Contributor

burke commented Feb 10, 2014

👍 from me too.

rafaelfranca added a commit that referenced this pull request Feb 10, 2014
@rafaelfranca rafaelfranca merged commit e063dbc into rails:master Feb 10, 2014
@jcoyne jcoyne deleted the fix_jruby_encoding branch February 10, 2014 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants