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

Performance optimizations #48

Merged
merged 10 commits into from
Jan 22, 2024
Merged

Performance optimizations #48

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e837507
Optimize performance of keys definition
Goltergaul Nov 22, 2023
7322c6b
Optimize performance of each definition
Goltergaul Nov 22, 2023
0f6255b
Optimize performance of and definition
Goltergaul Nov 22, 2023
d0614b9
Optimize performance of 'or' definition
Goltergaul Nov 22, 2023
ae1bc4a
Use instance variables instead of attr_accessors
Goltergaul Nov 28, 2023
68b8089
Improve nil definition perfromance
Goltergaul Nov 28, 2023
c1bd7ed
Update benchmark model
Goltergaul Nov 28, 2023
986fb47
Fix regression in and type
Goltergaul Nov 28, 2023
0a338c7
Add ruby 3.3 to ci
Goltergaul Jan 22, 2024
ffca06a
Update ruby version to 3.3.0
Goltergaul Jan 22, 2024
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
1 change: 1 addition & 0 deletions .ruby-version
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
3.2.2
4 changes: 4 additions & 0 deletions Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

## [1.1.0] - 2023-11-22
### Changes
- Improved performance

## [1.0.0] - 2023-02-14
### Removed
- Removed deprecated version of `GreaterThanEqual` definition that had a typo in it (GreaterThenEqual)
Expand Down
83 changes: 38 additions & 45 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,44 @@
PATH
remote: .
specs:
definition (1.0.0)
definition (1.1.0)
activesupport
i18n

GEM
remote: https://rubygems.org/
specs:
activesupport (6.1.7.3)
activesupport (7.1.2)
base64
bigdecimal
concurrent-ruby (~> 1.0, >= 1.0.2)
connection_pool (>= 2.2.5)
drb
i18n (>= 1.6, < 2)
minitest (>= 5.1)
mutex_m
tzinfo (~> 2.0)
zeitwerk (~> 2.3)
approvals (0.0.26)
json (~> 2.0)
nokogiri (~> 1.8)
thor (~> 1.0)
ast (2.4.2)
awesome_print (1.9.2)
benchmark-ips (2.10.0)
base64 (0.2.0)
benchmark-ips (2.12.0)
bigdecimal (3.1.4)
coderay (1.1.3)
concurrent-ruby (1.2.2)
connection_pool (2.4.1)
diff-lcs (1.5.0)
ffi (1.15.5)
ffi (1.15.5-java)
drb (2.2.0)
ruby2_keywords
ffi (1.16.3)
formatador (1.1.0)
fuubar (2.5.1)
rspec-core (~> 3.0)
ruby-progressbar (~> 1.4)
guard (2.18.0)
guard (2.18.1)
formatador (>= 0.2.4)
listen (>= 2.7, < 4.0)
lumberjack (>= 1.0.12, < 2.0)
Expand All @@ -44,66 +52,54 @@ GEM
guard (~> 2.1)
guard-compat (~> 1.1)
rspec (>= 2.99.0, < 4.0)
i18n (1.12.0)
i18n (1.14.1)
concurrent-ruby (~> 1.0)
jar-dependencies (0.4.1)
jaro_winkler (1.5.4)
jaro_winkler (1.5.4-java)
jaro_winkler (1.5.6)
json (2.6.3)
json (2.6.3-java)
listen (3.8.0)
rb-fsevent (~> 0.10, >= 0.10.3)
rb-inotify (~> 0.9, >= 0.9.10)
lumberjack (1.2.8)
lumberjack (1.2.10)
method_source (1.0.0)
mini_portile2 (2.8.1)
minitest (5.18.0)
minitest (5.20.0)
mutex_m (0.2.0)
nenv (0.3.0)
nokogiri (1.13.10)
mini_portile2 (~> 2.8.0)
racc (~> 1.4)
nokogiri (1.13.10-java)
nokogiri (1.15.5-x86_64-linux)
racc (~> 1.4)
notiffany (0.1.3)
nenv (~> 0.1)
shellany (~> 0.0)
parallel (1.22.1)
parser (3.2.1.0)
parallel (1.23.0)
parser (3.2.2.4)
ast (~> 2.4.1)
racc
pry (0.14.2)
coderay (~> 1.1)
method_source (~> 1.0)
pry (0.14.2-java)
coderay (~> 1.1)
method_source (~> 1.0)
spoon (~> 0.0)
psych (4.0.6)
psych (5.1.1.1)
stringio
psych (4.0.6-java)
jar-dependencies (>= 0.1.7)
racc (1.6.2)
racc (1.6.2-java)
racc (1.7.3)
rainbow (3.1.1)
rake (13.0.6)
rake (13.1.0)
rb-fsevent (0.11.2)
rb-inotify (0.10.1)
ffi (~> 1.0)
rspec (3.12.0)
rspec-core (~> 3.12.0)
rspec-expectations (~> 3.12.0)
rspec-mocks (~> 3.12.0)
rspec-core (3.12.1)
rspec-core (3.12.2)
rspec-support (~> 3.12.0)
rspec-expectations (3.12.2)
rspec-expectations (3.12.3)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.12.0)
rspec-its (1.3.0)
rspec-core (>= 3.0.0)
rspec-expectations (>= 3.0.0)
rspec-mocks (3.12.3)
rspec-mocks (3.12.6)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.12.0)
rspec-support (3.12.0)
rspec-support (3.12.1)
rspec_junit_formatter (0.6.0)
rspec-core (>= 2, < 4, != 2.12.0)
rubocop (0.66.0)
Expand All @@ -117,21 +113,18 @@ GEM
rubocop-rspec (1.32.0)
rubocop (>= 0.60.0)
rubocop_runner (2.2.1)
ruby-progressbar (1.11.0)
ruby-progressbar (1.13.0)
ruby2_keywords (0.0.5)
shellany (0.0.1)
spoon (0.0.6)
ffi
stringio (3.0.5)
thor (1.2.1)
timecop (0.9.6)
stringio (3.0.9)
thor (1.3.0)
timecop (0.9.8)
tzinfo (2.0.6)
concurrent-ruby (~> 1.0)
unicode-display_width (1.5.0)
zeitwerk (2.6.7)

PLATFORMS
ruby
universal-java-11
x86_64-linux

DEPENDENCIES
approvals (~> 0.0)
Expand All @@ -152,4 +145,4 @@ DEPENDENCIES
timecop

BUNDLED WITH
2.4.8
2.4.21
26 changes: 23 additions & 3 deletions benchmark/model.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

gemfile do
source "https://rubygems.org"
gem "dry-struct", "~> 1.4"
gem "dry-struct", "~> 1.6"
gem "awesome_print"
gem "benchmark-ips"
gem "pry"
Expand All @@ -21,6 +21,11 @@ class DryStructModel < Dry::Struct
attribute :app_name, Dry::Types["strict.string"].optional.default(nil)
attribute :app_branch, Dry::Types["strict.string"].optional.default(nil)
attribute :platform, Dry::Types["strict.string"].optional.default(nil)
attribute :user, Dry::Types["strict.hash"].schema(
name: Dry::Types["strict.string"],
age: Dry::Types["coercible.integer"]
)
attribute :array, Dry::Types["strict.array"].of(Dry::Types["strict.string"].enum("a", "b", "c", "d"))
end

class DefinitionModel < Definition::Model
Expand All @@ -30,10 +35,25 @@ class DefinitionModel < Definition::Model
optional :app_name, Definition.Type(String)
optional :app_branch, Definition.Type(String)
optional :platform, Definition.Type(String)
required(:user, Definition.Keys do
required :name, Definition.Type(String)
required :age, Definition.CoercibleType(Integer)
end)
optional :array, Definition.Each(Definition.Enum("a", "b", "c", "d"))
end

puts "Benchmark with valid input data:"
valid_data = { id: 1, app_key: "com.test", app_version: "1.0.0", app_name: "testapp" }
valid_data = {
id: 1,
app_key: "com.test",
app_version: "1.0.0",
app_name: "testapp",
user: {
name: "John Doe",
age: "65"
},
array: %w[a b c d a]
}

Benchmark.ips do |x|
x.config(time: 5, warmup: 2)
Expand All @@ -52,7 +72,7 @@ class DefinitionModel < Definition::Model
puts "Benchmark with invalid input data:"
invalid_data = { id: "abc", app_key: "com.test", app_name: "testapp" }
Benchmark.ips do |x|
x.config(time: 5, warmup: 2)
x.config(time: 20, warmup: 5)

x.report("definition") do
DefinitionModel.new(**invalid_data)
Expand Down
80 changes: 33 additions & 47 deletions lib/definition/types/each.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,64 +13,50 @@ def initialize(name, definition:)
super(name)
end

def conform(value)
Conformer.new(self).conform(value)
end
def conform(values)
return non_array_error(values) unless values.is_a?(Array)

def error_renderer
ErrorRenderers::Leaf
end
errors = false

class Conformer
def initialize(definition)
self.definition = definition
results = values.map do |value|
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no way to fail faster as you improved it for the and operation since we want to return all errors right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

hm yeah at least that is how it currently works 🤔 The error case is not so much the bottleneck though from what i've found. In error cases definition is already faster then dry-struct. The main performance issue is behind the all-is-valid case

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just an idea to try.
Instead of using map and building up an array with entries we probably don't need, we could also have two separte stacks for valid and invalid results like this:

results = []
faulty_results = []

values.each do |value|
  result = item_definition.conform(value)
          
  if result.passed?
    results << result
  else
    errors = true
    faulty_results << result
   end
end

return ConformResult.new(results.map(&:value)) unless errors

ConformResult.new(values, errors: [ConformError.new(self,
                                                            "Not all items conform with '#{name}'",
                                                            sub_errors: convert_errors(faulty_results))])

...

def convert_errors(faulty_results)
  errors = []
  faulty_results.each_with_index do |faulty_result, index|
    errors << KeyConformError.new(
      self,
      "Item #{faulty_result.value.inspect} did not conform to #{name}",
       key:        index,
       sub_errors: faulty_result.error_tree
       )
       end
      errors
end

Map uses push under the hood, which is a bit slower than <<.
Since we don't need all entries if there is an error, we don't need to have all individual results stored in the array.
By having a separate faulty_results stack, we can get rid of a check within convert_errors, too.

This version constantly performed a bit better on my machine.
Maybe give it a try and see if it's worth it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

just gave this a try with a slight adpatation, i still need all results for the convert_errors function, as this one needs to extract the correct index of the array elements for the error message.

with that i don't see a significant change in execution time. the benchmark results vary more from run to run than the difference it makes

Copy link
Owner Author

Choose a reason for hiding this comment

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

also with this the results are within error, seems even a bit slower:

def conform(values)
        return non_array_error(values) unless values.is_a?(Array)

        errors = []
        result_values = []

        values.each_with_index do |value, index|
          result = item_definition.conform(value)
          if result.passed?
            result_values << result.value
          else
            errors << KeyConformError.new(
            self,
            "Item #{result.value.inspect} did not conform to #{name}",
            key:        index,
            sub_errors: result.error_tree
          )
          end
        end

        return ConformResult.new(result_values) if errors.empty?

        ConformResult.new(values, errors: [ConformError.new(self,
                                                            "Not all items conform with '#{name}'",
                                                            sub_errors: errors)])
      end

result = item_definition.conform(value)
errors = true unless result.passed?
result
end

def conform(value)
return non_array_error(value) unless value.is_a?(Array)

results = conform_all(value)

if results.all?(&:conformed?)
ConformResult.new(results.map(&:value))
else
ConformResult.new(value, errors: [ConformError.new(definition,
"Not all items conform with '#{definition.name}'",
sub_errors: errors(results))])
end
end
return ConformResult.new(results.map(&:value)) unless errors

private
ConformResult.new(values, errors: [ConformError.new(self,
"Not all items conform with '#{name}'",
sub_errors: convert_errors(results))])
end

attr_accessor :definition
def error_renderer
ErrorRenderers::Leaf
end

def errors(results)
errors = []
results.each_with_index do |result, index|
next if result.passed?
private

errors << KeyConformError.new(
definition,
"Item #{result.value.inspect} did not conform to #{definition.name}",
key: index,
sub_errors: result.error_tree
)
end
errors
end
def convert_errors(results)
errors = []
results.each_with_index do |result, index|
next if result.passed?

def conform_all(values)
values.map do |value|
definition.item_definition.conform(value)
end
errors << KeyConformError.new(
self,
"Item #{result.value.inspect} did not conform to #{name}",
key: index,
sub_errors: result.error_tree
)
end
errors
end

def non_array_error(value)
ConformResult.new(value, errors: [
ConformError.new(definition,
"Non-Array value does not conform with #{definition.name}")
])
end
def non_array_error(value)
ConformResult.new(value, errors: [
ConformError.new(self,
"Non-Array value does not conform with #{name}")
])
end
end
end
Expand Down
Loading