Skip to content

Refactor validator strategies + Add ability to override validators #159

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

Jump to bottom
Merged
merged 2 commits into from
Jun 9, 2015
Merged

Refactor validator strategies + Add ability to override validators #159

merged 2 commits into from
Jun 9, 2015

Conversation

waterlink
Copy link
Collaborator

  • Extracted #call_with to its own module, without any changes to it.
  • Extracted + Refactored .make_validator in its own module.
  • Add ability to override validator strategies and add new ones (sort of, for user custom contracts)
  • Make all calls to Contract.make_validator faster by memoizing

fixes #150
fixes #156

Example code for overriding :class strategy:

require "rspec"
require "contracts"

Contract.override_validator(:class) do |contract|
  lambda do |arg|
    arg.is_a?(contract) || arg.is_a?(RSpec::Mocks::Double)
  end
end

@robnormal What do you think, can you use this kind of overriding mechanism?

/cc @egonSchiele Can you take a look at this one?

waterlink
waterlink reviewed Jun 9, 2015
View reviewed changes
lib/contracts/call_with.rb
@@ -0,0 +1,96 @@
module Contracts
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method was just moved.

@egonSchiele
Copy link
Owner

Neat! This is much better. Did you notice any speed difference with this change?

@waterlink
Copy link
Collaborator Author

There is no speed difference with our current benchmarks. But given a thought to it, I think this may cause a slowdown for Hash contract. I think I will add this benchmark here

@waterlink waterlink force-pushed the feature/override_validator branch from 554e81f to 13545c4 Compare June 9, 2015 22:29
@waterlink
Copy link
Collaborator Author

I have pushed benchmark itself right into master (no-op code).

master:

                                     user     system      total        real
testing add                      1.350000   0.000000   1.350000 (  1.360404)
testing contracts add           12.650000   0.000000  12.650000 ( 12.683449)
MethodProfiler results for: Contract
+------------------------+----------+----------+--------------+-------------+-------------+
| Method                 | Min Time | Max Time | Average Time | Total Time  | Total Calls |
+------------------------+----------+----------+--------------+-------------+-------------+
| #call_with             | 0.598 ms | 7.528 ms | 0.742 ms     | 7418.086 ms | 10000       |
| .valid?                | 0.048 ms | 6.801 ms | 0.069 ms     | 1376.411 ms | 20000       |
| #maybe_append_options! | 0.047 ms | 0.685 ms | 0.064 ms     | 642.044 ms  | 10000       |
| .make_validator        | 0.004 ms | 6.179 ms | 0.007 ms     | 138.552 ms  | 20000       |
| #failure_exception     | 0.003 ms | 0.555 ms | 0.005 ms     | 54.535 ms   | 10000       |
| #method                | 0.003 ms | 0.615 ms | 0.005 ms     | 90.243 ms   | 20000       |
| #ret_contract          | 0.003 ms | 0.512 ms | 0.004 ms     | 44.013 ms   | 10000       |
| #maybe_append_block!   | 0.003 ms | 0.162 ms | 0.004 ms     | 40.909 ms   | 10000       |
| #args_contracts        | 0.003 ms | 0.182 ms | 0.004 ms     | 80.797 ms   | 20000       |
+------------------------+----------+----------+--------------+-------------+-------------+
MethodProfiler results for: Object
+-----------------------------------------------------+----------+----------+--------------+-------------+-------------+
| Method                                              | Min Time | Max Time | Average Time | Total Time  | Total Calls |
+-----------------------------------------------------+----------+----------+--------------+-------------+-------------+
| #contracts_add                                      | 0.710 ms | 7.652 ms | 0.872 ms     | 8717.126 ms | 10000       |
| #__contracts_ruby_original_contracts_add_iapwejs... | 0.004 ms | 0.782 ms | 0.006 ms     | 55.634 ms   | 10000       |
| .__contracts_engine                                 | 0.004 ms | 0.037 ms | 0.005 ms     | 0.484 ms    | 97          |
+-----------------------------------------------------+----------+----------+--------------+-------------+-------------+
MethodProfiler results for: Contracts::MethodDecorators
+-------------------------+----------+----------+--------------+------------+-------------+
| Method                  | Min Time | Max Time | Average Time | Total Time | Total Calls |
+-------------------------+----------+----------+--------------+------------+-------------+
| #method_added           | 0.066 ms | 0.600 ms | 0.144 ms     | 6.044 ms   | 42          |
| #singleton_method_added | 0.065 ms | 0.111 ms | 0.087 ms     | 0.786 ms   | 9           |
+-------------------------+----------+----------+--------------+------------+-------------+
MethodProfiler results for: Contracts::Decorator
MethodProfiler results for: Contracts::Support
+----------------+----------+----------+--------------+------------+-------------+
| Method         | Min Time | Max Time | Average Time | Total Time | Total Calls |
+----------------+----------+----------+--------------+------------+-------------+
| .eigenclass_of | 0.004 ms | 0.034 ms | 0.006 ms     | 0.225 ms   | 39          |
+----------------+----------+----------+--------------+------------+-------------+
MethodProfiler results for: UnboundMethod

This branch:

                                     user     system      total        real
testing add                      1.360000   0.000000   1.360000 (  1.365564)
testing contracts add           14.880000   0.000000  14.880000 ( 14.929929)
MethodProfiler results for: Contract
+------------------------+----------+----------+--------------+------------+-------------+
| Method                 | Min Time | Max Time | Average Time | Total Time | Total Calls |
+------------------------+----------+----------+--------------+------------+-------------+
| #maybe_append_options! | 0.047 ms | 0.686 ms | 0.059 ms     | 590.857 ms | 10000       |
| .valid?                | 0.007 ms | 0.555 ms | 0.009 ms     | 183.756 ms | 20000       |
| #failure_exception     | 0.003 ms | 0.526 ms | 0.005 ms     | 45.985 ms  | 10000       |
| #maybe_append_block!   | 0.003 ms | 0.813 ms | 0.004 ms     | 44.937 ms  | 10000       |
| #method                | 0.003 ms | 0.583 ms | 0.004 ms     | 77.914 ms  | 20000       |
| #ret_contract          | 0.003 ms | 0.518 ms | 0.004 ms     | 38.197 ms  | 10000       |
| #args_contracts        | 0.003 ms | 0.569 ms | 0.004 ms     | 74.744 ms  | 20000       |
+------------------------+----------+----------+--------------+------------+-------------+
MethodProfiler results for: Object
+-----------------------------------------------------+----------+----------+--------------+-------------+-------------+
| Method                                              | Min Time | Max Time | Average Time | Total Time  | Total Calls |
+-----------------------------------------------------+----------+----------+--------------+-------------+-------------+
| #contracts_add                                      | 0.551 ms | 6.971 ms | 0.637 ms     | 6365.304 ms | 10000       |
| #__contracts_ruby_original_contracts_add_iapwhm8... | 0.004 ms | 0.526 ms | 0.005 ms     | 47.899 ms   | 10000       |
| .__contracts_engine                                 | 0.004 ms | 0.012 ms | 0.005 ms     | 0.457 ms    | 97          |
+-----------------------------------------------------+----------+----------+--------------+-------------+-------------+
MethodProfiler results for: Contracts::MethodDecorators
+-------------------------+----------+----------+--------------+------------+-------------+
| Method                  | Min Time | Max Time | Average Time | Total Time | Total Calls |
+-------------------------+----------+----------+--------------+------------+-------------+
| #method_added           | 0.062 ms | 0.694 ms | 0.142 ms     | 5.946 ms   | 42          |
| #singleton_method_added | 0.065 ms | 0.119 ms | 0.085 ms     | 0.762 ms   | 9           |
+-------------------------+----------+----------+--------------+------------+-------------+
MethodProfiler results for: Contracts::Decorator
MethodProfiler results for: Contracts::Support
+----------------+----------+----------+--------------+------------+-------------+
| Method         | Min Time | Max Time | Average Time | Total Time | Total Calls |
+----------------+----------+----------+--------------+------------+-------------+
| .eigenclass_of | 0.004 ms | 0.038 ms | 0.005 ms     | 0.194 ms   | 39          |
+----------------+----------+----------+--------------+------------+-------------+
MethodProfiler results for: UnboundMethod

Yes, as expected there is a bit of slowdown here. All contracts that call to Contract.valid? under the hood are affected by this.
I have one idea, how to optimize, I will try it out and post the results.

@waterlink
Copy link
Collaborator Author

Ok, now it is even faster than master (I actually expected that) :)

                                     user     system      total        real
testing add                      1.360000   0.000000   1.360000 (  1.365206)
testing contracts add           10.890000   0.000000  10.890000 ( 10.962997)
MethodProfiler results for: Contract
+------------------------+----------+----------+--------------+-------------+-------------+
| Method                 | Min Time | Max Time | Average Time | Total Time  | Total Calls |
+------------------------+----------+----------+--------------+-------------+-------------+
| .valid?                | 0.047 ms | 0.860 ms | 0.062 ms     | 1230.053 ms | 20000       |
| #maybe_append_options! | 0.047 ms | 1.541 ms | 0.060 ms     | 602.754 ms  | 10000       |
| #maybe_append_block!   | 0.003 ms | 0.699 ms | 0.005 ms     | 47.537 ms   | 10000       |
| #failure_exception     | 0.003 ms | 0.747 ms | 0.005 ms     | 46.856 ms   | 10000       |
| #ret_contract          | 0.003 ms | 0.697 ms | 0.004 ms     | 41.659 ms   | 10000       |
| #method                | 0.003 ms | 0.538 ms | 0.004 ms     | 79.677 ms   | 20000       |
| #args_contracts        | 0.003 ms | 0.696 ms | 0.004 ms     | 78.764 ms   | 20000       |
+------------------------+----------+----------+--------------+-------------+-------------+
MethodProfiler results for: Object
+-----------------------------------------------------+----------+----------+--------------+-------------+-------------+
| Method                                              | Min Time | Max Time | Average Time | Total Time  | Total Calls |
+-----------------------------------------------------+----------+----------+--------------+-------------+-------------+
| #contracts_add                                      | 0.653 ms | 7.254 ms | 0.745 ms     | 7445.788 ms | 10000       |
| .__contracts_engine                                 | 0.004 ms | 0.032 ms | 0.005 ms     | 0.507 ms    | 97          |
| #__contracts_ruby_original_contracts_add_iapwu41... | 0.004 ms | 0.703 ms | 0.005 ms     | 50.130 ms   | 10000       |
+-----------------------------------------------------+----------+----------+--------------+-------------+-------------+
MethodProfiler results for: Contracts::MethodDecorators
+-------------------------+----------+----------+--------------+------------+-------------+
| Method                  | Min Time | Max Time | Average Time | Total Time | Total Calls |
+-------------------------+----------+----------+--------------+------------+-------------+
| #method_added           | 0.068 ms | 1.431 ms | 0.175 ms     | 7.343 ms   | 42          |
| #singleton_method_added | 0.067 ms | 0.136 ms | 0.081 ms     | 0.728 ms   | 9           |
+-------------------------+----------+----------+--------------+------------+-------------+
MethodProfiler results for: Contracts::Decorator
MethodProfiler results for: Contracts::Support
+----------------+----------+----------+--------------+------------+-------------+
| Method         | Min Time | Max Time | Average Time | Total Time | Total Calls |
+----------------+----------+----------+--------------+------------+-------------+
| .eigenclass_of | 0.004 ms | 0.010 ms | 0.005 ms     | 0.192 ms   | 39          |
| .contract_id   | 0.003 ms | 0.575 ms | 0.004 ms     | 83.964 ms  | 20000       |
+----------------+----------+----------+--------------+------------+-------------+
MethodProfiler results for: UnboundMethod

@waterlink
Copy link
Collaborator Author

Contracts that don't call to Contract.valid? are unaffected.

@waterlink waterlink force-pushed the feature/override_validator branch from 693c9a0 to 88375aa Compare June 9, 2015 22:45
@waterlink
Copy link
Collaborator Author

Green on travis and faster than on master :) What can be better? :D

@egonSchiele
Copy link
Owner

oh, awesome!

@egonSchiele
Copy link
Owner

merging :)

egonSchiele added a commit that referenced this pull request Jun 9, 2015
@egonSchiele
Merge pull request #159 from waterlink/feature/override_validator
8c1c5a6 
Refactor validator strategies + Add ability to override validators
@egonSchiele egonSchiele merged commit 8c1c5a6 into egonSchiele:master Jun 9, 2015
@waterlink waterlink deleted the feature/override_validator branch June 9, 2015 23:01
@robnormal
Copy link
Contributor

Excellent!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use code generation for inlining
3 participants
@waterlink @egonSchiele @robnormal