From d8052fdee07411d0aff122740df0bc89f2e657cb Mon Sep 17 00:00:00 2001 From: nvh0412 Date: Tue, 7 Oct 2025 17:43:54 +1100 Subject: [PATCH 1/4] chore: reduce objection allocations from requires and optional DSL Current approach: Always clones attrs and opts (in optional), then mutates and clones again Optimized approach: remove redundant dup by writing our own extract_options! without mutating the input --- lib/grape/dsl/parameters.rb | 37 ++++++++++++++++++++----------- spec/grape/dsl/parameters_spec.rb | 21 +++++++++++++++++- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/lib/grape/dsl/parameters.rb b/lib/grape/dsl/parameters.rb index b4e6154da..0595d8f3c 100644 --- a/lib/grape/dsl/parameters.rb +++ b/lib/grape/dsl/parameters.rb @@ -124,17 +124,17 @@ def use(*names) # end # end def requires(*attrs, &block) - orig_attrs = attrs.clone - - opts = attrs.extract_options!.clone + # Extract options without mutating the original attrs array + attrs_clean, opts = extract_options_non_mutating(attrs) + opts = opts.clone opts[:presence] = { value: true, message: opts[:message] } opts = @group.deep_merge(opts) if instance_variable_defined?(:@group) && @group if opts[:using] - require_required_and_optional_fields(attrs.first, opts) + require_required_and_optional_fields(attrs_clean.first, opts) else - validate_attributes(attrs, opts, &block) - block ? new_scope(orig_attrs, &block) : push_declared_params(attrs, opts.slice(:as)) + validate_attributes(attrs_clean, opts, &block) + block ? new_scope(attrs, &block) : push_declared_params(attrs_clean, opts.slice(:as)) end end @@ -143,24 +143,23 @@ def requires(*attrs, &block) # @param (see #requires) # @option (see #requires) def optional(*attrs, &block) - orig_attrs = attrs.clone - - opts = attrs.extract_options!.clone + # Extract options without mutating the original attrs array + attrs_clean, opts = extract_options_non_mutating(attrs) type = opts[:type] opts = @group.deep_merge(opts) if instance_variable_defined?(:@group) && @group # check type for optional parameter group - if attrs && block + if attrs_clean && block raise Grape::Exceptions::MissingGroupType if type.nil? raise Grape::Exceptions::UnsupportedGroupType unless Grape::Validations::Types.group?(type) end if opts[:using] - require_optional_fields(attrs.first, opts) + require_optional_fields(attrs_clean.first, opts) else - validate_attributes(attrs, opts, &block) + validate_attributes(attrs_clean, opts, &block) - block ? new_scope(orig_attrs, true, &block) : push_declared_params(attrs, opts.slice(:as)) + block ? new_scope(attrs, true, &block) : push_declared_params(attrs_clean, opts.slice(:as)) end end @@ -256,6 +255,18 @@ def params(params) private + # Extract options from an array without mutating the original array. + # This is more memory-efficient than cloning the array and then using extract_options! + # @param attrs [Array] the attributes array that may contain a hash as the last element + # @return [Array] a tuple of [clean_attrs, options_hash] + def extract_options_non_mutating(attrs) + if attrs.last.is_a?(Hash) + [attrs[0...-1], attrs.last] + else + [attrs, {}] + end + end + def first_hash_key_or_param(parameter) parameter.is_a?(Hash) ? parameter.keys.first : parameter end diff --git a/spec/grape/dsl/parameters_spec.rb b/spec/grape/dsl/parameters_spec.rb index 065811a82..c1077ced1 100644 --- a/spec/grape/dsl/parameters_spec.rb +++ b/spec/grape/dsl/parameters_spec.rb @@ -99,8 +99,17 @@ def extract_message_option(attrs) expect(subject.validate_attributes_reader).to eq([[:id], { type: Integer, desc: 'Identity.', presence: { value: true, message: nil } }]) expect(subject.push_declared_params_reader).to eq([:id]) end - end + it 'preserves original attrs array when calling new_scope' do + attrs = [:id, type: Array, desc: 'Identity.'] + original_attrs = attrs.dup + + expect(subject).to receive(:new_scope).with(attrs) + subject.requires(*attrs) {[]} + + expect(attrs).to eq(original_attrs) + end + end describe '#optional' do it 'adds an optional parameter' do subject.optional :id, type: Integer, desc: 'Identity.' @@ -108,6 +117,16 @@ def extract_message_option(attrs) expect(subject.validate_attributes_reader).to eq([[:id], { type: Integer, desc: 'Identity.' }]) expect(subject.push_declared_params_reader).to eq([:id]) end + + it 'preserves original attrs array when calling new_scope' do + attrs = [:id, { type: Array, desc: 'Identity.' }] + original_attrs = attrs.dup + + expect(subject).to receive(:new_scope).with(attrs, true) + subject.optional(*attrs) {[]} + + expect(attrs).to eq(original_attrs) + end end describe '#with' do From 695a607eda748b00fd73544e80e4ac89f24083dd Mon Sep 17 00:00:00 2001 From: nvh0412 Date: Tue, 7 Oct 2025 22:49:51 +1100 Subject: [PATCH 2/4] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2126b0a5e..70f10e0ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ * [#2594](https://github.com/ruby-grape/grape/pull/2594): Fix routes memoization - [@ericproulx](https://github.com/ericproulx). * [#2595](https://github.com/ruby-grape/grape/pull/2595): Keep `within_namespace` as part of our internal api - [@ericproulx](https://github.com/ericproulx). * [#2596](https://github.com/ruby-grape/grape/pull/2596): Remove `namespace_reverse_stackable_with_hash` from public scope - [@ericproulx](https://github.com/ericproulx). +* [#2611](https://github.com/ruby-grape/grape/pull/2611): Reduce objection allocations from requires and optional dsl - [@nvh0412](https://github.com/nvh0412) * Your contribution here. ### 2.4.0 (2025-06-18) From ae96122b41d38af587d26a4c28183cb94a8c6198 Mon Sep 17 00:00:00 2001 From: nvh0412 Date: Tue, 7 Oct 2025 22:55:18 +1100 Subject: [PATCH 3/4] Fix lintings --- spec/grape/dsl/parameters_spec.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/grape/dsl/parameters_spec.rb b/spec/grape/dsl/parameters_spec.rb index c1077ced1..a75fd6dfa 100644 --- a/spec/grape/dsl/parameters_spec.rb +++ b/spec/grape/dsl/parameters_spec.rb @@ -101,15 +101,16 @@ def extract_message_option(attrs) end it 'preserves original attrs array when calling new_scope' do - attrs = [:id, type: Array, desc: 'Identity.'] + attrs = [:id, { type: Array, desc: 'Identity.' }] original_attrs = attrs.dup expect(subject).to receive(:new_scope).with(attrs) - subject.requires(*attrs) {[]} + subject.requires(*attrs) { [] } expect(attrs).to eq(original_attrs) end end + describe '#optional' do it 'adds an optional parameter' do subject.optional :id, type: Integer, desc: 'Identity.' @@ -123,7 +124,7 @@ def extract_message_option(attrs) original_attrs = attrs.dup expect(subject).to receive(:new_scope).with(attrs, true) - subject.optional(*attrs) {[]} + subject.optional(*attrs) { [] } expect(attrs).to eq(original_attrs) end From 2745ea033e43109eefa466fe112d8b26b52ceadb Mon Sep 17 00:00:00 2001 From: nvh0412 Date: Tue, 7 Oct 2025 23:51:46 +1100 Subject: [PATCH 4/4] Correct changelog format --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70f10e0ee..a7eb8780a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,7 @@ * [#2594](https://github.com/ruby-grape/grape/pull/2594): Fix routes memoization - [@ericproulx](https://github.com/ericproulx). * [#2595](https://github.com/ruby-grape/grape/pull/2595): Keep `within_namespace` as part of our internal api - [@ericproulx](https://github.com/ericproulx). * [#2596](https://github.com/ruby-grape/grape/pull/2596): Remove `namespace_reverse_stackable_with_hash` from public scope - [@ericproulx](https://github.com/ericproulx). -* [#2611](https://github.com/ruby-grape/grape/pull/2611): Reduce objection allocations from requires and optional dsl - [@nvh0412](https://github.com/nvh0412) +* [#2611](https://github.com/ruby-grape/grape/pull/2611): Reduce objection allocations from requires and optional dsl - [@nvh0412](https://github.com/nvh0412). * Your contribution here. ### 2.4.0 (2025-06-18)