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

Optimize JSON::Pure::Generator by 2x-4x for simple options cases #586

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

eregon
Copy link
Member

@eregon eregon commented May 9, 2024

* ruby --yjit benchmarks/bench.rb dump pure ruby 3.3.1 (2024-04-23 revision c56cd86388) +YJIT [x86_64-linux]
  Before: JSON.dump(obj)    604.604  (± 0.3%) i/s    (1.65 ms/i) -      3.060k in   5.061200s
  After:  JSON.dump(obj)      2.531k (± 0.4%) i/s  (395.14 μs/i) -     12.801k in   5.058326s
* ruby benchmarks/bench.rb dump pure
  truffleruby 24.1.0-dev-a8ebb51b, like ruby 3.2.2, Oracle GraalVM JVM [x86_64-linux]
  Before: JSON.dump(obj)      3.728k (± 9.4%) i/s  (268.26 μs/i) -     18.559k in   5.068915s
  After:  JSON.dump(obj)      7.835k (± 8.5%) i/s  (127.63 μs/i) -     39.004k in   5.031116s

Using the benchmark from #580

BTW, the C extension, with YJIT gives:

JSON::Ext::Generator
ruby 3.3.1 (2024-04-23 revision c56cd86388) +YJIT [x86_64-linux]
JSON.dump(obj)      7.402k (± 1.0%) i/s  (135.10 μs/i) -     37.485k in   5.064832s

@eregon eregon force-pushed the optimize-ruby-generator branch 3 times, most recently from 0dee779 to 4e87543 Compare May 9, 2024 16:15
@eregon
Copy link
Member Author

eregon commented Jun 3, 2024

@hsbt Could you review this as well? 🙏


cc @byroot I mentioned this to you at RubyKaigi.
Might also be interesting to @maximecb (this is a pure-Ruby JSON parser, this PR is optimizing the pure-Ruby JSON generator).

Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

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

Some minor comments, but looks good.

Comment on lines +291 to +294
if @indent.empty? and @space.empty? and @space_before.empty? and @object_nl.empty? and @array_nl.empty? and
!@ascii_only and !@script_safe and @max_nesting == 0 and !@strict
Copy link
Member

Choose a reason for hiding this comment

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

This feels like this could be precomputed in configure so you'd only need to check a single ivar, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmh, I'm not sure what's the protocol, the State class has a bunch of public attr_accessor so it looks like configure is not necessarily called after such accessors would be used to write.
Or maybe JSON users are not supposed to call these accessors? What are they for then?

BTW it seems pretty inefficient that e.g. JSON.generate would create a new State every time (which runs the State constructor and configure every time) instead of using SAFE_STATE_PROTOTYPE.dup or so, but that's a separate thing so improve later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Out of curiosity I did

diff --git a/lib/json/pure/generator.rb b/lib/json/pure/generator.rb
index 9f44ef5..d07c2de 100644
--- a/lib/json/pure/generator.rb
+++ b/lib/json/pure/generator.rb
@@ -268,6 +268,10 @@ module JSON
           else
             @max_nesting = 0
           end
+
+          @fast = (@indent.empty? and @space.empty? and @space_before.empty? and @object_nl.empty? and @array_nl.empty? and
+            !@ascii_only and !@script_safe and @max_nesting == 0 and !@strict)
+
           self
         end
         alias merge configure
@@ -277,6 +281,7 @@ module JSON
         def to_h
           result = {}
           instance_variables.each do |iv|
+            next if iv == :@fast
             iv = iv.to_s[1..-1]
             result[iv.to_sym] = self[iv]
           end
@@ -292,8 +297,10 @@ module JSON
         def generate(obj)
           if @indent.empty? and @space.empty? and @space_before.empty? and @object_nl.empty? and @array_nl.empty? and
               !@ascii_only and !@script_safe and @max_nesting == 0 and !@strict
+            raise "OOPS" if !@fast
             result = generate_json(obj, '')
           else
+            raise "OOPS" if @fast
             result = obj.to_json(self)
           end
           JSON.valid_utf8?(result) or raise GeneratorError,

To see if there were any inconsistency and the test suite didn't find any.
But still, I think we need to understand better how these accessors are used (or maybe remove them if they're not needed).

buf << '"'.freeze
end
else
# Ruby 2.3 compatibility
Copy link
Member

Choose a reason for hiding this comment

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

At this point we might consider bumping the required version to 2.4 or even 2.5

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be nice, but this is not much overhead, so that could be cleaned up later when we bump the min Ruby version.

lib/json/pure/generator.rb Outdated Show resolved Hide resolved
* ruby --yjit benchmarks/bench.rb dump pure
  ruby 3.3.1 (2024-04-23 revision c56cd86388) +YJIT [x86_64-linux]
  Before: JSON.dump(obj)    604.604  (± 0.3%) i/s    (1.65 ms/i) -      3.060k in   5.061200s
  After:  JSON.dump(obj)      2.531k (± 0.4%) i/s  (395.14 μs/i) -     12.801k in   5.058326s
* ruby benchmarks/bench.rb dump pure
  truffleruby 24.1.0-dev-a8ebb51b, like ruby 3.2.2, Oracle GraalVM JVM [x86_64-linux]
  Before: JSON.dump(obj)      3.728k (± 9.4%) i/s  (268.26 μs/i) -     18.559k in   5.068915s
  After:  JSON.dump(obj)      7.835k (± 8.5%) i/s  (127.63 μs/i) -     39.004k in   5.031116s
@eregon
Copy link
Member Author

eregon commented Oct 8, 2024

Rebased and CI should be green

@maximecb maximecb merged commit 1c0615a into ruby:master Oct 8, 2024
73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants