From 4905995b4f739c004c9a300eccd84ef8537d0b57 Mon Sep 17 00:00:00 2001 From: Dmitriy Nesteryuk Date: Sat, 5 Oct 2019 15:22:19 +0300 Subject: [PATCH] micro optimizations in allocating hashes and arrays There were a few places where hashes could be mutated without side effects, they already work with copies of original hashes. So, `merge!` replaced some of `merge` calls. Also, `Grape::Util::StackableValues`, `Grape::Util::ReverseStackableValues` and `Grape::Util::InheritableValues` got a base class which keeps common methods to avoid duplication. Besides, that a `[]` and `keys` methods were optimized to allocate less arrays when it is possible. There is an additional benchmark which measures more complex scenario. Unfortunately, there isn't noticeable difference in performance between the master and this change. However, I also profiled the same endpoint as the benchmark has. Before this change: Measure Mode: memory Thread ID: 47216803207600 Fiber ID: 47216805628400 Total: 40536.000000 Sort by: self_time %self total self wait child calls name 13.85 6848.000 5616.000 0.000 1232.000 15 *Hash#each_pair 9.73 3944.000 3944.000 0.000 0.000 17 Hash#merge 8.80 5088.000 3568.000 0.000 1520.000 8 *Array#map 8.57 11720.000 3472.000 0.000 8248.000 27 *Class#new 4.54 1840.000 1840.000 0.000 0.000 46 Symbol#to_s 4.44 1800.000 1800.000 0.000 0.000 20 *Grape::Util::StackableValues#[] 3.36 1440.000 1360.000 0.000 80.000 5 Grape::Endpoint#run_filters 2.88 3040.000 1168.000 0.000 1872.000 9 *Hash#each 2.37 1200.000 960.000 0.000 240.000 6 #normalize_path 2.29 1088.000 928.000 0.000 160.000 4 ActiveSupport::HashWithIndifferentAccess#[]= 2.21 1128.000 896.000 0.000 232.000 8 *Kernel#dup 2.07 840.000 840.000 0.000 0.000 9 String#split 1.64 12008.000 664.000 0.000 11344.000 1 Grape::Endpoint#run_validators After this change: Measure Mode: memory Thread ID: 47390769740200 Fiber ID: 47390772121400 Total: 37296.000000 Sort by: self_time %self total self wait child calls name 15.06 6848.000 5616.000 0.000 1232.000 15 *Hash#each_pair 9.57 5088.000 3568.000 0.000 1520.000 8 *Array#map 9.31 11720.000 3472.000 0.000 8248.000 27 *Class#new 4.93 1840.000 1840.000 0.000 0.000 46 Symbol#to_s 4.35 1624.000 1624.000 0.000 0.000 7 Hash#merge 3.65 1440.000 1360.000 0.000 80.000 5 Grape::Endpoint#run_filters 3.13 3040.000 1168.000 0.000 1872.000 9 *Hash#each 2.57 1200.000 960.000 0.000 240.000 6 #normalize_path 2.49 1088.000 928.000 0.000 160.000 4 ActiveSupport::HashWithIndifferentAccess#[]= 2.40 1128.000 896.000 0.000 232.000 8 *Kernel#dup 2.25 840.000 840.000 0.000 0.000 9 String#split 2.15 1400.000 800.000 0.000 600.000 20 *Grape::Util::StackableValues#[] 1.78 11768.000 664.000 0.000 11104.000 1 Grape::Endpoint#run_validators - The total memory usage went down from 40536 to 37296 bytes. - `Hash#merge` moved from second position to 5th. - `Grape::Util::StackableValues#[]` moved from 6th to 12th. --- CHANGELOG.md | 1 + benchmark/nested_params.rb | 41 +++++++++++++++++++++ lib/grape/endpoint.rb | 2 +- lib/grape/error_formatter.rb | 2 +- lib/grape/exceptions/validation_errors.rb | 6 ++- lib/grape/formatter.rb | 2 +- lib/grape/parser.rb | 2 +- lib/grape/util/base_inheritable.rb | 34 +++++++++++++++++ lib/grape/util/inheritable_values.rb | 30 +++------------ lib/grape/util/reverse_stackable_values.rb | 43 ++++------------------ lib/grape/util/stackable_values.rb | 41 ++++++++++----------- lib/grape/validations/validators/base.rb | 8 ++-- 12 files changed, 119 insertions(+), 93 deletions(-) create mode 100644 benchmark/nested_params.rb create mode 100644 lib/grape/util/base_inheritable.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index f6eb770b6..a01bdafff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ #### Features * Your contribution here. +* [#1915](https://github.com/ruby-grape/grape/pull/1915): Micro optimizations in allocating hashes and arrays - [@dnesteryuk](https://github.com/dnesteryuk). * [#1904](https://github.com/ruby-grape/grape/pull/1904): Allows Grape to load files on startup rather than on the first call - [@myxoh](https://github.com/myxoh). * [#1907](https://github.com/ruby-grape/grape/pull/1907): Adds outside configuration to Grape with `configure` - [@unleashy](https://github.com/unleashy). diff --git a/benchmark/nested_params.rb b/benchmark/nested_params.rb new file mode 100644 index 000000000..1e4f88133 --- /dev/null +++ b/benchmark/nested_params.rb @@ -0,0 +1,41 @@ +require 'grape' +require 'benchmark/ips' + +class API < Grape::API + prefix :api + version 'v1', using: :path + + params do + requires :address, type: Hash do + requires :street, type: String + requires :postal_code, type: Integer + optional :city, type: String + end + end + post '/' do + 'hello' + end +end + +options = { + method: 'POST', + params: { + address: { + street: 'Alexis Pl.', + postal_code: '90210', + city: 'Beverly Hills' + } + } +} + +env = Rack::MockRequest.env_for('/api/v1', options) + +10.times do |i| + env["HTTP_HEADER#{i}"] = '123' +end + +Benchmark.ips do |ips| + ips.report('POST with nested params') do + API.call env + end +end diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb index 12b2150ff..0551ef88d 100644 --- a/lib/grape/endpoint.rb +++ b/lib/grape/endpoint.rb @@ -200,7 +200,7 @@ def prepare_version end def merge_route_options(**default) - options[:route_options].clone.merge(**default) + options[:route_options].clone.merge!(**default) end def map_routes diff --git a/lib/grape/error_formatter.rb b/lib/grape/error_formatter.rb index f63e8e510..85107f6e1 100644 --- a/lib/grape/error_formatter.rb +++ b/lib/grape/error_formatter.rb @@ -14,7 +14,7 @@ def builtin_formatters end def formatters(options) - builtin_formatters.merge(default_elements).merge(options[:error_formatters] || {}) + builtin_formatters.merge(default_elements).merge!(options[:error_formatters] || {}) end def formatter_for(api_format, **options) diff --git a/lib/grape/exceptions/validation_errors.rb b/lib/grape/exceptions/validation_errors.rb index 92bf5d590..822a5fce4 100644 --- a/lib/grape/exceptions/validation_errors.rb +++ b/lib/grape/exceptions/validation_errors.rb @@ -39,14 +39,16 @@ def to_json(**_opts) end def full_messages - map { |attributes, error| full_message(attributes, error) }.uniq + messages = map { |attributes, error| full_message(attributes, error) } + messages.uniq! + messages end private def full_message(attributes, error) I18n.t( - 'grape.errors.format'.to_sym, + 'grape.errors.format', default: '%{attributes} %{message}', attributes: attributes.count == 1 ? translate_attribute(attributes.first) : translate_attributes(attributes), message: error.message diff --git a/lib/grape/formatter.rb b/lib/grape/formatter.rb index 5047e722a..ef95fcb7a 100644 --- a/lib/grape/formatter.rb +++ b/lib/grape/formatter.rb @@ -14,7 +14,7 @@ def builtin_formmaters end def formatters(options) - builtin_formmaters.merge(default_elements).merge(options[:formatters] || {}) + builtin_formmaters.merge(default_elements).merge!(options[:formatters] || {}) end def formatter_for(api_format, **options) diff --git a/lib/grape/parser.rb b/lib/grape/parser.rb index 41ad889eb..a1e87bb40 100644 --- a/lib/grape/parser.rb +++ b/lib/grape/parser.rb @@ -12,7 +12,7 @@ def builtin_parsers end def parsers(options) - builtin_parsers.merge(default_elements).merge(options[:parsers] || {}) + builtin_parsers.merge(default_elements).merge!(options[:parsers] || {}) end def parser_for(api_format, **options) diff --git a/lib/grape/util/base_inheritable.rb b/lib/grape/util/base_inheritable.rb new file mode 100644 index 000000000..277c61d5b --- /dev/null +++ b/lib/grape/util/base_inheritable.rb @@ -0,0 +1,34 @@ +module Grape + module Util + # Base for classes which need to operate with own values kept + # in the hash and inherited values kept in a Hash-like object. + class BaseInheritable + attr_accessor :inherited_values + attr_accessor :new_values + + # @param inherited_values [Object] An object implementing an interface + # of the Hash class. + def initialize(inherited_values = {}) + @inherited_values = inherited_values + @new_values = {} + end + + def delete(key) + new_values.delete key + end + + def initialize_copy(other) + super + self.inherited_values = other.inherited_values + self.new_values = other.new_values.dup + end + + def keys + combined = inherited_values.keys + combined.concat(new_values.keys) + combined.uniq! + combined + end + end + end +end diff --git a/lib/grape/util/inheritable_values.rb b/lib/grape/util/inheritable_values.rb index 2546075db..bba7be02b 100644 --- a/lib/grape/util/inheritable_values.rb +++ b/lib/grape/util/inheritable_values.rb @@ -1,14 +1,8 @@ +require_relative 'base_inheritable' + module Grape module Util - class InheritableValues - attr_accessor :inherited_values - attr_accessor :new_values - - def initialize(inherited_values = {}) - self.inherited_values = inherited_values - self.new_values = {} - end - + class InheritableValues < BaseInheritable def [](name) values[name] end @@ -17,26 +11,12 @@ def []=(name, value) new_values[name] = value end - def delete(key) - new_values.delete key - end - def merge(new_hash) - values.merge(new_hash) - end - - def keys - (new_values.keys + inherited_values.keys).sort.uniq + values.merge!(new_hash) end def to_hash - values.clone - end - - def initialize_copy(other) - super - self.inherited_values = other.inherited_values - self.new_values = other.new_values.dup + values end protected diff --git a/lib/grape/util/reverse_stackable_values.rb b/lib/grape/util/reverse_stackable_values.rb index 474325f72..8858c83fe 100644 --- a/lib/grape/util/reverse_stackable_values.rb +++ b/lib/grape/util/reverse_stackable_values.rb @@ -1,45 +1,16 @@ +require_relative 'stackable_values' + module Grape module Util - class ReverseStackableValues - attr_accessor :inherited_values - attr_accessor :new_values - - def initialize(inherited_values = {}) - @inherited_values = inherited_values - @new_values = {} - end + class ReverseStackableValues < StackableValues + protected - def [](name) + def concat_values(inherited_value, new_value) [].tap do |value| - value.concat(@new_values[name] || []) - value.concat(@inherited_values[name] || []) - end - end - - def []=(name, value) - @new_values[name] ||= [] - @new_values[name].push value - end - - def delete(key) - new_values.delete key - end - - def keys - (@new_values.keys + @inherited_values.keys).sort.uniq - end - - def to_hash - keys.each_with_object({}) do |key, result| - result[key] = self[key] + value.concat(new_value) + value.concat(inherited_value) end end - - def initialize_copy(other) - super - self.inherited_values = other.inherited_values - self.new_values = other.new_values.dup - end end end end diff --git a/lib/grape/util/stackable_values.rb b/lib/grape/util/stackable_values.rb index a6c8179bd..4c97adace 100644 --- a/lib/grape/util/stackable_values.rb +++ b/lib/grape/util/stackable_values.rb @@ -1,23 +1,25 @@ +require_relative 'base_inheritable' + module Grape module Util - class StackableValues - attr_accessor :inherited_values - attr_accessor :new_values + class StackableValues < BaseInheritable attr_reader :frozen_values - def initialize(inherited_values = {}) - @inherited_values = inherited_values - @new_values = {} + def initialize(*_args) + super + @frozen_values = {} end def [](name) return @frozen_values[name] if @frozen_values.key? name - value = [] - value.concat(@inherited_values[name] || []) - value.concat(@new_values[name] || []) - value + inherited_value = @inherited_values[name] + new_value = @new_values[name] || [] + + return new_value unless inherited_value + + concat_values(inherited_value, new_value) end def []=(name, value) @@ -26,14 +28,6 @@ def []=(name, value) @new_values[name].push value end - def delete(key) - new_values.delete key - end - - def keys - (@new_values.keys + @inherited_values.keys).sort.uniq - end - def to_hash keys.each_with_object({}) do |key, result| result[key] = self[key] @@ -44,10 +38,13 @@ def freeze_value(key) @frozen_values[key] = self[key].freeze end - def initialize_copy(other) - super - self.inherited_values = other.inherited_values - self.new_values = other.new_values.dup + protected + + def concat_values(inherited_value, new_value) + [].tap do |value| + value.concat(inherited_value) + value.concat(new_value) + end end end end diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index 169e2155a..c62956acc 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -56,10 +56,10 @@ def validate!(params) def self.convert_to_short_name(klass) ret = klass.name.gsub(/::/, '/') - .gsub(/([A-Z]+)([A-Z][a-z])/, '\1_\2') - .gsub(/([a-z\d])([A-Z])/, '\1_\2') - .tr('-', '_') - .downcase + ret.gsub!(/([A-Z]+)([A-Z][a-z])/, '\1_\2') + ret.gsub!(/([a-z\d])([A-Z])/, '\1_\2') + ret.tr!('-', '_') + ret.downcase! File.basename(ret, '_validator') end