Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Make sure we only collect binary encoded strings in our buffer. #10

Merged
merged 1 commit into from Jan 12, 2017

Conversation

arthurschreiber
Copy link
Member

@charliesome this should fix the issue encountered by you.


This changes the internal Buffer class to convert all strings written into it to have a binary/raw encoding instead of whatever encoding the string originally had. When the strings get written to the wire their encoding is lost (or transmitted separately in V2 of the protocol) anyway.


Non-scientific benchmarks:

Before:

$ bundle exec ruby bench/bench.rb 
       user     system      total        real
BERT tiny  0.010000   0.000000   0.010000 (  0.006823)
BERT small  0.060000   0.000000   0.060000 (  0.057645)
BERT large  0.290000   0.110000   0.400000 (  0.406172)
BERT complex  1.150000   0.010000   1.160000 (  1.150710)

After:

$ bundle exec ruby bench/bench.rb 
       user     system      total        real
BERT tiny  0.010000   0.000000   0.010000 (  0.007411)
BERT small  0.060000   0.000000   0.060000 (  0.064392)
BERT large  0.280000   0.030000   0.310000 (  0.308900)
BERT complex  1.270000   0.000000   1.270000 (  1.273539)

@carlosmn
Copy link
Collaborator

It looks like String#b does not exist in the default ruby on Travis (which seems to be 1.9.3 right now). Do you reckon it'd be worth adding a Travis file and testing in 2.0 upwards (or whatever the supported version is currently?)

@arthurschreiber
Copy link
Member Author

@charliesome already did that in the production+travis branch: https://github.com/github/bert/blob/production%2Btravis/.travis.yml

So once all these things get merged together, everything should be working fine. 🤞

@haileys haileys merged commit 83ffa7f into github:master Jan 12, 2017
@haileys
Copy link

haileys commented Jan 12, 2017

Thanks @arthurschreiber!

@haileys haileys mentioned this pull request Jan 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants