search

brandur / json_schema

A JSON Schema V4 and Hyperschema V4 parser and validator.
MIT License
230 stars 45 forks source link

implement fail-fast option #96

Closed breunigs closed 6 years ago

breunigs commented 6 years ago

I experimented quite a bit with a decent fail fast solution. There's two tricky parts:

  1. all validation methods that work on groups need special handling (allOf, anyOf and oneOf)
  2. keeping the speed in the non-fail fast case

Doing any kind of meta programming in the and method or similar makes the normal case 1/3rd slower, at least in all the ideas I tried. Trying to work without throw/catch makes the code rather unwieldy, at which point I think the speedup is not worth the additional maintenance effort anymore.

This branch has the following timings:

 raw timing in master          difference   which version
 9,629  9,348  9,574  9,805    100%         master
10,095  9,855  9,845 10,205    104%         fail slowly, no meta programming (first commit)
 9,684  9,366  9,418  9,564    100.85%      fail slowly, with meta programming (both commits)
 7,607  7,669  7,710  7,913     80%         fail fast (just tested with first commit, with both it should also be slightly faster)

I have not looked into details if the .85 percentage points for the "fail slowly w/ meta" case are by chance or if's due to the extra work of having to define the method in the beginning. I think with all the other improvements, this loss of performance might be acceptable.

If you're generally fine with this approach, I'll also amend the readme to cover this additional capability.

breunigs commented 6 years ago

Thank you for the review, I think your suggestions make a lot of sense. Can you take a peek if the comment for the meta programming are understandable? I was also not quite sure what you meant by spacing out, but I hope my changes make it easier to read?

brandur commented 6 years ago

Sure thing. Please note that I will have to drop 1.9 support then, though.

One other possible option is to use the old school def foo(..., options = {}); options[:fail_fast] ... syntax to get 1.9-compatible syntax that looks like optional named parameters.

That said, I just looked and 1.9's been EOL'ed for over three years now (Feb 2015), so I think it's time. I was previously keeping 1.9 support around because this gem was a transitive dependency for stripe-ruby, which supported 1.9, but we dropped 1.9 there, so it's safe to drop it here too.

Thank you for the review, I think your suggestions make a lot of sense. Can you take a peek if the comment for the meta programming are understandable?

Awesome. Looks great! Thanks for the quick turnaround.

I was also not quite sure what you meant by spacing out, but I hope my changes make it easier to read?

I just meant that we should put an empty line above and below so that the code isn't quite so dense. You've already done it here, so all good!

brandur commented 6 years ago

Released as 0.19.0.