Redux: speed up JSON::Validator.validate #285
Conversation
|
If I can get this reviewed I'd like to release as v2.6.0 |
iainbeeston
force-pushed
the
cache-normalized-uri
branch
from
2d36afe to
78412a6
Compare
| end | ||
| temp_uri.fragment = "" if temp_uri.fragment.nil? | ||
| temp_uri = temp_uri.merge(:fragment => "") if temp_uri.fragment.nil? || temp_uri.fragment.empty? |
There was a problem hiding this comment.
If temp_uri.fragment.empty? is true, then will temp_uri.merge(fragment: "") actually change anything?
Also, can do temp_uri.merge!(fragment: "") instead of temp_uri = temp_uri.merge(fragment: "") to be consistent with preceding code.
There was a problem hiding this comment.
Let me answer those in turn:
- If temp_uri.fragment is "", then the URI will have a trailing "#" symbol. If temp_uri.fragment is nil or empty then no "#" is appended. So doing this does change the behaviour
- We can't use merge! here, because is temp_uri.relative? is false, then temp_uri has come directly from the cache in JSON::Util:URI, and uris stored in the cache are frozen. If you call merge! on a frozen uri it will raise an exception because you are modifying a frozen object. By using merge it will work with both branches of the code
iainbeeston
force-pushed
the
cache-normalized-uri
branch
2 times, most recently
from
19d49e1 to
95f9176
Compare
iainbeeston
force-pushed
the
cache-normalized-uri
branch
2 times, most recently
from
ef36232 to
3881475
Compare
|
I've refactored slightly to duplicate cached URIs rather than freezing them |
|
(Because in addressable dup is cheaper than freeze) |
| @@ -71,8 +70,6 @@ def read(location) | |||
| JSON::Schema.new(JSON::Validator.parse(body), uri) | |||
| end | |||
|
|
|||
| # @param uri [Addressable::URI] | |||
| # @return [Boolean] | |||
There was a problem hiding this comment.
Why delete the docstrings?
There was a problem hiding this comment.
Because in 6967d99 I've tried to abstract away the fact that we use Addressable::URI, however, I'm probably kidding myself. So I've restored this until I have time to make a more water-tight solution.
Addressible's URI parsing shows up at the top of the profiler for our use case. This patch adds a cache to JSON::Util::URI.normalized_uri, to trade CPU time for memory. There are other expensive Addressible methods that should be cached elsewhere. benchmark: https://github.com/mjc/json-schema-perf MRI 2.2.2 before: 825.363 (± 6.2%) i/s MRI 2.2.2 after: 1.436k (± 4.5%) i/s 1.74x faster JRuby 1.7.20 before: 1.445k (± 2.5%) i/s JRuby 1.7.20 after: 2.272k (± 6.1%) i/s 1.57x faster
Addressable::URI#fragment= triggers a revalidation of the URL, which is super slow. Don't do this when it isn't necessary.
Hopefully this will make it easier to change in future
iainbeeston
force-pushed
the
cache-normalized-uri
branch
from
3881475 to
6967d99
Compare
|
Looks fine to me 👍 |
|
Thanks. I'll merge this and make a release
|
Redux: speed up JSON::Validator.validate
This is a rebase and update of #255
I've pulled out the change to the way uuids are calculated (in #255 the uuid of a Hash is calculated by taking a SHA of
Hash.hash, whereas in the original the SHA of the string of the hash is used) because I feel that the risk is high and a better solution would be to use a faster hashing algorithm.However, I've expanded the use of caching to get around the speed problems in Addressable.
Worst case this should be faster than the current implementation.