Use rb_enc_interned_str if available

[casperisfine]

Ref: https://bugs.ruby-lang.org/issues/13381

So if all goes well, Ruby 3.0 should expose rb_enc_interned_str(char* ptr, long len, rb_encoding* enc). This function returns an interned string like String#-@, except that if the string already exist, it can return it without any allocation.

So for documents with a lot of repeated object keys, or using the freeze => true option, this has the potential to very heavily cut down on the number of allocation.

NB: the function doesn't work well on current ruby master, but hopefully ruby/ruby#3786 will make it usable. But I'd like to start the discussion now if possible.

[casperisfine]

ruby/ruby#3786 was merged which means rb_enc_interned_str should be usable in Ruby 3.0.

@hsbt are you interested in this PR?

[casperisfine]

@hsbt could I interest you in this PR, or @marcandre maybe?

[marcandre]

Looks like a great PR, but I don't have write access. I wish we move this to ruby/json.

[casperisfine]

but I don't have write access

Yeah I know :/

Who else could I try to pick their interest on this?

[hsbt]

I'm not sure what advantage with this change.

@nurse Do you interest in this?

[marcandre]

It should be more efficient. Just to be sure: interned strings may be garbage collected like symbols?

[nurse]

It should be more efficient. Just to be sure: interned strings may be garbage collected like symbols?

It will be garbage collected when there's not references to the fstring: https://github.com/ruby/ruby/blob/55e52c19e74d5df90560ea1cc4f2a2b9f5d7a5c4/string.c#L1410-L1419

1e95cf1 Refactor json_string_unescape

How large this improve the performance? If it improves enough performance, see review comments.

[casperisfine]

I'm not sure what advantage with this change.

As discussed in https://bugs.ruby-lang.org/issues/16029, we have large configuration files we parse on boot and keep in memory forever. They contains a lot of repeated keys, that end up allocated and deduplicated later. This change would reduce our allocations on boot by over 20%, and since we spend 25-30% of our boot time in the GC, I'd expect a relatively nice speedup for us here.

Concretely, the following JSON:

[
  {"foo": 1},
  {"foo": 2},
]

Will currently allocate "foo" twice, but then likely GC both of them because Hash keys are automatically interned. By using rb_enc_interned_str we'd save two allocations.

Just to be sure: interned strings may be garbage collected like symbols?

Yes. It's similar to String#-@, it simply store them in some kind of internal weakmap to allow reusing them, but if they are dangling they'll be cleaned up.

How large this improve the performance?

If the numbers I posted above aren't enough I can try to craft a benchmark.

[casperisfine]

How large this improve the performance?

Unless you meant the json_unescape refactoring itself? Because the reason I refactor it is that to prevent allocation, rb_enc_interned_str takes a char *. So the escaping has to be done on char * rather than on a Ruby String.

[nurse]

Unless you meant the json_unescape refactoring itself? Because the reason I refactor it is that to prevent allocation, rb_enc_interned_str takes a char *. So the escaping has to be done on char * rather than on a Ruby String.

Yes, and I know. But as far as I understand these days just creating Ruby object is not so heavy while they are collected by Generational GC. I want to ensure the refactoring is worth to add more code complexity. Micro bench mark is ok, but also want one which tests alloca and xmalloc (shorter strings and longer strings).

[casperisfine]

Ok. I'll try to produce theses soon.

[casperisfine]

Ok, so first micro benchmark, to see if allocating on the stack is worth it:

# frozen_string_literals: true
require 'benchmark/ips'
require 'json'
puts "Ruby: #{RUBY_VERSION}"
json = JSON.dump("\n\r\t\\")

Benchmark.ips do |x|
  x.report('parse escaped') { JSON.parse(json) }
end

This PR:

$ ruby -Ilib:ext/ /tmp/bench-json.rb 
Ruby: 2.7.2
Warming up --------------------------------------
       parse escaped   117.556k i/100ms
Calculating -------------------------------------
       parse escaped      1.178M (± 1.3%) i/s -      5.995M in   5.088271s
$ ruby -Ilib:ext/ /tmp/bench-json.rb 
Ruby: 2.7.2
Warming up --------------------------------------
       parse escaped   116.897k i/100ms
Calculating -------------------------------------
       parse escaped      1.154M (± 1.4%) i/s -      5.845M in   5.064481s

With same but with the condition commented out (so always using xmalloc):

$ ruby -Ilib:ext/ /tmp/bench-json.rb 
Ruby: 2.7.2
Warming up --------------------------------------
       parse escaped   110.132k i/100ms
Calculating -------------------------------------
       parse escaped      1.107M (± 1.8%) i/s -      5.617M in   5.074198s
$ ruby -Ilib:ext/ /tmp/bench-json.rb 
Ruby: 2.7.2
Warming up --------------------------------------
       parse escaped   110.173k i/100ms
Calculating -------------------------------------
       parse escaped      1.092M (± 1.5%) i/s -      5.509M in   5.045623s

So the difference doesn't seem that huge, but seem consistent. If you think it is a useless optimization, I'm fine with removing it.

[casperisfine]

Oh, I forgot you asked for larger strings as well:

# frozen_string_literals: true
require 'benchmark/ips'
require 'json'
puts "Ruby: #{RUBY_VERSION}"
json = JSON.dump("\n\r\t\\")
json2 = JSON.dump("\n\r\t\\" * 16)

Benchmark.ips do |x|
  x.report('parse escaped(10B)') { JSON.parse(json) }
  x.report('parse escaped(130B)') { JSON.parse(json2) }
end

This PR:

$ ruby -Ilib:ext/ /tmp/bench-json.rb 
Ruby: 2.7.2
Warming up --------------------------------------
  parse escaped(10B)   116.996k i/100ms
 parse escaped(130B)    73.702k i/100ms
Calculating -------------------------------------
  parse escaped(10B)      1.169M (± 0.9%) i/s -      5.850M in   5.006243s
 parse escaped(130B)    736.164k (± 1.2%) i/s -      3.685M in   5.006539s
$ ruby -Ilib:ext/ /tmp/bench-json.rb 
Ruby: 2.7.2
Warming up --------------------------------------
  parse escaped(10B)   117.741k i/100ms
 parse escaped(130B)    74.573k i/100ms
Calculating -------------------------------------
  parse escaped(10B)      1.175M (± 1.7%) i/s -      5.887M in   5.010103s
 parse escaped(130B)    741.524k (± 1.2%) i/s -      3.729M in   5.029060s

With alloca removed:

$ ruby -Ilib:ext/ /tmp/bench-json.rb 
Ruby: 2.7.2
Warming up --------------------------------------
  parse escaped(10B)   109.443k i/100ms
 parse escaped(130B)    66.379k i/100ms
Calculating -------------------------------------
  parse escaped(10B)      1.101M (± 1.0%) i/s -      5.582M in   5.070559s
 parse escaped(130B)    668.632k (± 1.3%) i/s -      3.385M in   5.063923s
$ ruby -Ilib:ext/ /tmp/bench-json.rb 
Ruby: 2.7.2
Warming up --------------------------------------
  parse escaped(10B)   109.493k i/100ms
 parse escaped(130B)    66.244k i/100ms
Calculating -------------------------------------
  parse escaped(10B)      1.093M (± 1.0%) i/s -      5.475M in   5.009441s
 parse escaped(130B)    667.558k (± 1.2%) i/s -      3.378M in   5.061627s

[casperisfine]

Another benchmark, so see the need for this feature overall:

# frozen_string_literal: true
require 'benchmark/ips'
require 'json'
puts "Ruby: #{RUBY_VERSION}"
json = JSON.dump(
  'foo' => 'bar',
  'plopplopplopplopplopplopplopplopplopplopplopplopplopplopplop' => 12,
  'bar' => 'foo',
  'egg' => 'spam',
  'spam' => 'egg',
)

Benchmark.ips do |x|
  x.report('parse(freeze: true)') { JSON.parse(json, freeze: true) }
end

This time on ruby 3.0.0 (to have rb_enc_interned_str):

8f7ddba (before this PR):

$ ruby -Ilib:ext/ /tmp/bench-interned-str.rb 
Ruby: 3.0.0
Warming up --------------------------------------
 parse(freeze: true)    27.322k i/100ms
Calculating -------------------------------------
 parse(freeze: true)    271.591k (± 1.8%) i/s -      1.366M in   5.031762s
$ ruby -Ilib:ext/ /tmp/bench-interned-str.rb 
Ruby: 3.0.0
Warming up --------------------------------------
 parse(freeze: true)    27.437k i/100ms
Calculating -------------------------------------
 parse(freeze: true)    275.807k (± 1.3%) i/s -      1.399M in   5.074361s

This PR:

$ ruby -Ilib:ext/ /tmp/bench-interned-str.rb 
Ruby: 3.0.0
Warming up --------------------------------------
 parse(freeze: true)    39.884k i/100ms
Calculating -------------------------------------
 parse(freeze: true)    399.643k (± 1.5%) i/s -      2.034M in   5.090945s
$ ruby -Ilib:ext/ /tmp/bench-interned-str.rb 
Ruby: 3.0.0
Warming up --------------------------------------
 parse(freeze: true)    40.338k i/100ms
Calculating -------------------------------------
 parse(freeze: true)    400.744k (± 1.5%) i/s -      2.017M in   5.034074s

[casperisfine]

@nurse if these benchmark are good enough for you I can work on rebasing and solving your comments.

[nurse]

@casperisfine Thank you for the benchmark. It looks this PR improves so much and alloca is also worth to introduce additional complexity. I'll merge this when you fix this.

[casperisfine]

@nurse I moved the rb_str_intern call inside json_string_unescape as requested. However I can't use rb_sym_intern as it's not public.

[nurse]

@nurse I moved the rb_str_intern call inside json_string_unescape as requested. However I can't use rb_sym_intern as it's not public.

Oops sorry indeed. Please ignore that.

[casperisfine]

I just noticed travis isn't testing against Ruby 3.0. I can push that if you'd like.

[casperisfine]

@nurse is there anything else I could do?

[nurse]

Some CIs are failed and how about 38e8477#r553682398?

[casperisfine]

Some CIs are failed

Indeed, but I don't see any output. I don't know why they failed. I'll see if I can retry the jobs.

how about 38e8477#r553682398?

Not sure what this links to. It point to and old commit but I see no comment.

[casperisfine]

I'll see if I can retry the jobs.

Seems like I don't have the permission to retry the jobs, and they are all very specific versions of OSX, test fully pass on ubuntu. I suspect some kind of flakiness, but no way to tell without the output.

[casperisfine]

how about 38e8477#r553682398?

Ok, based on the comment id I think you are referring to ALLOC_N / ALLOCA_N. I've fixed that a while ago.

I edited my last commit message to try to restart CI, we'll see how it goes.

[casperisfine]

So all the macos- job simply won't start. I suppose GitHub doesn't have enough macos capacity.

[nurse]

@casperisfine thanks! I merged this! Merged commit's CI works fine.

[casperisfine]

Thank you!