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

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

Merged
merged 1 commit into from
Oct 8, 2024
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 61 additions & 1 deletion lib/json/pure/generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,8 @@ def configure(opts)
opts.each do |key, value|
instance_variable_set "@#{key}", value
end

# NOTE: If adding new instance variables here, check whether #generate should check them for #generate_json
@indent = opts[:indent] if opts.key?(:indent)
@space = opts[:space] if opts.key?(:space)
@space_before = opts[:space_before] if opts.key?(:space_before)
Expand Down Expand Up @@ -288,12 +290,70 @@ def to_h
# created this method raises a
# GeneratorError exception.
def generate(obj)
result = obj.to_json(self)
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
Comment on lines +293 to +294
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).

result = generate_json(obj, '')
else
result = obj.to_json(self)
end
JSON.valid_utf8?(result) or raise GeneratorError,
"source sequence #{result.inspect} is illegal/malformed utf-8"
result
end

# Handles @allow_nan, @buffer_initial_length, other ivars must be the default value (see above)
private def generate_json(obj, buf)
case obj
when Hash
buf << '{'.freeze
first = true
eregon marked this conversation as resolved.
Show resolved Hide resolved
obj.each_pair do |k,v|
buf << ','.freeze unless first
fast_serialize_string(k.to_s, buf)
buf << ':'.freeze
generate_json(v, buf)
first = false
end
buf << '}'.freeze
when Array
buf << '['.freeze
first = true
obj.each do |e|
buf << ','.freeze unless first
generate_json(e, buf)
first = false
end
buf << ']'.freeze
when String
fast_serialize_string(obj, buf)
when Integer
buf << obj.to_s
else
# Note: Float is handled this way since Float#to_s is slow anyway
buf << obj.to_json(self)
end
end

# Assumes !@ascii_only, !@script_safe
if Regexp.method_defined?(:match?)
private def fast_serialize_string(string, buf) # :nodoc:
buf << '"'.freeze
string = string.encode(::Encoding::UTF_8) unless string.encoding == ::Encoding::UTF_8

if /["\\\x0-\x1f]/n.match?(string)
buf << string.gsub(/["\\\x0-\x1f]/n, MAP)
else
buf << string
end
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.

private def fast_serialize_string(string, buf) # :nodoc:
buf << string.to_json(self)
end
end

# Return the value returned by method +name+.
def [](name)
if respond_to?(name)
Expand Down
Loading