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

Handle non-Hash to Hash transformation in before(:key_coercer) #362

Merged
merged 1 commit into from
Jun 29, 2021

Conversation

ojab
Copy link
Contributor

@ojab ojab commented Jun 13, 2021

Extract from #353.
Fixes #350

Instead of passing only part of the output to the scoped results we're passing whole output and path as a scope, this was result can replace scoped value without any assumptions on the initial and replacement types.

Performance for the common case (steps execution only on the outer schema) is not affected, running benchmarks/params_valid_vs_invalid.rb (10 times each because of the noise)
Before:

       valid input 8:    32011.2 i/s
       valid input 0:    31832.2 i/s - same-ish: difference falls within error
       valid input 7:    31779.7 i/s - same-ish: difference falls within error
       valid input 1:    31690.8 i/s - same-ish: difference falls within error
       valid input 2:    31623.7 i/s - same-ish: difference falls within error
       valid input 4:    31438.5 i/s - same-ish: difference falls within error
       valid input 6:    31369.0 i/s - same-ish: difference falls within error
       valid input 5:    31180.7 i/s - same-ish: difference falls within error
       valid input 3:    31118.2 i/s - same-ish: difference falls within error
       valid input 9:    30681.5 i/s - same-ish: difference falls within error
     invalid input 9:    20153.8 i/s - 1.59x  (± 0.00) slower
     invalid input 8:    20103.2 i/s - 1.59x  (± 0.00) slower
     invalid input 7:    20067.7 i/s - 1.60x  (± 0.00) slower
     invalid input 1:    19941.1 i/s - 1.61x  (± 0.00) slower
     invalid input 2:    19937.2 i/s - 1.61x  (± 0.00) slower
     invalid input 3:    19563.4 i/s - 1.64x  (± 0.00) slower
     invalid input 0:    19548.0 i/s - 1.64x  (± 0.00) slower
     invalid input 6:    19313.2 i/s - 1.66x  (± 0.00) slower
     invalid input 5:    18576.3 i/s - 1.72x  (± 0.00) slower
     invalid input 4:    17281.0 i/s - 1.85x  (± 0.00) slower

After:

Comparison:
       valid input 0:    32465.3 i/s
       valid input 2:    32141.4 i/s - same-ish: difference falls within error
       valid input 3:    31842.5 i/s - same-ish: difference falls within error
       valid input 5:    31600.6 i/s - same-ish: difference falls within error
       valid input 6:    31525.6 i/s - same-ish: difference falls within error
       valid input 4:    31306.8 i/s - same-ish: difference falls within error
       valid input 7:    31173.7 i/s - same-ish: difference falls within error
       valid input 8:    31123.6 i/s - same-ish: difference falls within error
       valid input 9:    30858.5 i/s - same-ish: difference falls within error
       valid input 1:    30832.5 i/s - same-ish: difference falls within error
     invalid input 5:    20427.4 i/s - 1.59x  (± 0.00) slower
     invalid input 4:    20413.8 i/s - 1.59x  (± 0.00) slower
     invalid input 6:    20385.7 i/s - 1.59x  (± 0.00) slower
     invalid input 7:    20368.8 i/s - 1.59x  (± 0.00) slower
     invalid input 9:    20276.1 i/s - 1.60x  (± 0.00) slower
     invalid input 8:    20184.7 i/s - 1.61x  (± 0.00) slower
     invalid input 1:    20084.0 i/s - 1.62x  (± 0.00) slower
     invalid input 2:    20078.9 i/s - 1.62x  (± 0.00) slower
     invalid input 3:    19898.7 i/s - 1.63x  (± 0.00) slower
     invalid input 0:    19893.5 i/s - 1.63x  (± 0.00) slower

dry-v specs are passing, didn't measured performance of the nested steps.

@@ -34,7 +29,7 @@ class Result
option :message_compiler

# @api private
option :parent, default: -> { nil }
option :path, optional: true, reader: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer default: -> { Path::EMPTY }, but we're losing 5% on this:

       valid input 1:    30483.6 i/s
       valid input 6:    30419.9 i/s - same-ish: difference falls within error
       valid input 7:    30366.9 i/s - same-ish: difference falls within error
       valid input 0:    30279.9 i/s - same-ish: difference falls within error
       valid input 5:    30050.6 i/s - same-ish: difference falls within error
       valid input 3:    29944.3 i/s - same-ish: difference falls within error
       valid input 8:    29839.7 i/s - same-ish: difference falls within error
       valid input 9:    29766.8 i/s - same-ish: difference falls within error
       valid input 2:    29754.7 i/s - same-ish: difference falls within error
       valid input 4:    29656.9 i/s - same-ish: difference falls within error
     invalid input 4:    19371.6 i/s - 1.57x  (± 0.00) slower
     invalid input 1:    19308.5 i/s - 1.58x  (± 0.00) slower
     invalid input 0:    19255.6 i/s - 1.58x  (± 0.00) slower
     invalid input 7:    19084.0 i/s - 1.60x  (± 0.00) slower
     invalid input 9:    19066.2 i/s - 1.60x  (± 0.00) slower
     invalid input 5:    19024.8 i/s - 1.60x  (± 0.00) slower
     invalid input 6:    18973.9 i/s - 1.61x  (± 0.00) slower
     invalid input 8:    18956.7 i/s - 1.61x  (± 0.00) slower
     invalid input 3:    18102.1 i/s - 1.68x  (± 0.00) slower
     invalid input 2:    18000.0 i/s - 1.69x  (± 0.00) slower

Copy link
Contributor Author

@ojab ojab Jun 13, 2021

Choose a reason for hiding this comment

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

heh, getting rid of dry-initializer

--- a/lib/dry/schema/result.rb
+++ b/lib/dry/schema/result.rb
@@ -1,6 +1,5 @@
 # frozen_string_literal: true
 
-require "dry/initializer"
 require "dry/core/equalizer"
 
 require "dry/schema/path"
@@ -15,21 +14,14 @@ module Dry
     class Result
       include Dry::Equalizer(:output, :errors)
 
-      extend Dry::Initializer[undefined: false]
+      attr_reader :message_compiler, :result_ast, :path
 
-      # @api private
-      param :output, reader: false
-
-      # A list of failure ASTs produced by rule result objects
-      #
-      # @api private
-      option :result_ast, default: -> { EMPTY_ARRAY.dup }
-
-      # @api private
-      option :message_compiler
-
-      # @api private
-      option :path, optional: true, reader: false
+      def initialize(output, result_ast: nil, message_compiler:, path: Path::EMPTY)
+        @output = output
+        @result_ast = result_ast || EMPTY_ARRAY.dup
+        @message_compiler = message_compiler
+        @path = path
+      end
 
       # @api private
       def self.new(*, **)
@@ -66,11 +58,6 @@ module Dry
         self
       end
 
-      # @api private
-      def path
-        @path || Path::EMPTY
-      end
-
       # Dump result to a hash returning processed and validated data
       #
       # @return [Hash]

leads to

       valid input 9:    49575.0 i/s
       valid input 3:    49255.8 i/s - same-ish: difference falls within error
       valid input 5:    49251.2 i/s - same-ish: difference falls within error
       valid input 6:    48890.9 i/s - same-ish: difference falls within error
       valid input 4:    48641.2 i/s - same-ish: difference falls within error
       valid input 7:    48640.9 i/s - same-ish: difference falls within error
       valid input 0:    48610.4 i/s - same-ish: difference falls within error
       valid input 8:    48258.1 i/s - same-ish: difference falls within error
       valid input 1:    48055.8 i/s - same-ish: difference falls within error
       valid input 2:    47932.6 i/s - same-ish: difference falls within error
     invalid input 8:    27419.9 i/s - 1.81x  (± 0.00) slower
     invalid input 9:    27418.2 i/s - 1.81x  (± 0.00) slower
     invalid input 2:    27125.1 i/s - 1.83x  (± 0.00) slower
     invalid input 1:    27051.1 i/s - 1.83x  (± 0.00) slower
     invalid input 0:    27025.2 i/s - 1.83x  (± 0.00) slower
     invalid input 7:    26545.8 i/s - 1.87x  (± 0.00) slower
     invalid input 3:    25785.0 i/s - 1.92x  (± 0.00) slower
     invalid input 6:    23449.3 i/s - 2.11x  (± 0.00) slower
     invalid input 4:    23342.6 i/s - 2.12x  (± 0.00) slower
     invalid input 5:    22610.3 i/s - 2.19x  (± 0.00) slower

Copy link
Member

Choose a reason for hiding this comment

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

@ojab so, it's faster with dry-initializer, am I reading this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, 32k/19k with dry-initializer, 49k/27k without.

Copy link
Member

Choose a reason for hiding this comment

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

@ojab oh damn this is a gigantic difference. We could replace it in a separate PR because it'd be a standalone performance improvement. BTW I'd like to re-implement dry-initializer as a simpler API in dry-core eventually. It's a very useful API that we have in a ton of gems, including rom-rb, but it could be simplified internally and most likely it would become faster, too.

@ojab ojab force-pushed the fixup_non_hash_handling branch 2 times, most recently from 6ec7464 to afdaea9 Compare June 13, 2021 13:40
@@ -100,6 +100,8 @@ def last
def same_root?(other)
root.equal?(other.root)
end

EMPTY = new([]).freeze
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not ideal placement, but dunno where else should I put it.

Copy link
Member

Choose a reason for hiding this comment

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

Super minor but maybe EMPTY = new(EMPTY_ARRAY).freeze?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ojab ojab marked this pull request as ready for review June 14, 2021 09:18
@ojab ojab requested a review from solnic as a code owner June 14, 2021 09:18
@@ -100,6 +100,8 @@ def last
def same_root?(other)
root.equal?(other.root)
end

EMPTY = new([]).freeze
Copy link
Member

Choose a reason for hiding this comment

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

Super minor but maybe EMPTY = new(EMPTY_ARRAY).freeze?

@@ -34,7 +29,7 @@ class Result
option :message_compiler

# @api private
option :parent, default: -> { nil }
option :path, optional: true, reader: false
Copy link
Member

Choose a reason for hiding this comment

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

@ojab so, it's faster with dry-initializer, am I reading this right?

@solnic solnic merged commit 9d70409 into dry-rb:master Jun 29, 2021
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.

Processor steps callbacks for nested schema are called in wrong order
2 participants