Recursion guard

[samuelcolvin]

This is a first, very naive attempt.

It seem to simple - am I missing something?

@adriangb please let me know what you think.

Needs:

• more tests
• check how bad the performance penalty is - seems to be pretty small ~2% for the complete benchmark
• experiment with making recursion_guard a struct with None by default until it's used
• maybe check with pyo3 that obj.as_ptr() as usize is a legitimate way to do id(obj)

[codecov]

Codecov Report

Merging #134 (6f41b71) into main (fdd0b3b) will increase coverage by 0.12%.
The diff coverage is 98.63%.

@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
+ Coverage   97.31%   97.44%   +0.12%     
==========================================
  Files          40       41       +1     
  Lines        3916     4068     +152     
  Branches       28       28              
==========================================
+ Hits         3811     3964     +153     
+ Misses        105      104       -1     

Impacted Files	Coverage Δ
src/errors/kinds.rs	100.00% <ø> (ø)
src/lib.rs	100.00% <ø> (ø)
src/validators/dict.rs	94.33% <88.88%> (+0.16%)	⬆️
src/recursion_guard.rs	94.11% <94.11%> (ø)
src/errors/validation_exception.rs	95.45% <96.29%> (+0.71%)	⬆️
src/errors/line_error.rs	97.11% <100.00%> (+0.27%)	⬆️
src/input/input_abstract.rs	100.00% <100.00%> (ø)
src/input/input_python.rs	98.22% <100.00%> (+0.01%)	⬆️
src/input/return_enums.rs	95.32% <100.00%> (+0.27%)	⬆️
src/validators/any.rs	100.00% <100.00%> (ø)
... and 23 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdd0b3b...6f41b71. Read the comment docs.

[adriangb]

If I understand correctly, you basically have a Set[int] that you are passing into each validator. The int is the id(obj) of each python object. Seems legit to me!

And yes agreed that ideally you'd make it an Option<RecursionGuard> and create it in validate() in recursive.rs since it isn't necessary if there is not recursive validator.

Have you considered using https://github.com/paritytech/nohash-hasher since you're using python's id as the key?

[samuelcolvin]

Have you considered using https://github.com/paritytech/nohash-hasher since you're using python's id as the key?

Interesting, I'll see if it's faster than ahash, I guess it must be.

[samuelcolvin]

You were right samuelcolvin/rust-bench@95b1062

test rust_set_a_hash_set        ... bench:         755 ns/iter (+/- 8)
test rust_set_btree_set         ... bench:       2,019 ns/iter (+/- 43)
test rust_set_hash_set          ... bench:       2,551 ns/iter (+/- 11)
test rust_set_no_hash_set       ... bench:         480 ns/iter (+/- 12)

[adriangb]

nice! 2x as fast, and 5x faster than the standard alg

[samuelcolvin]

I'm going to merge this, it might not be perfect but we can improve if someone can find a case which breaks it.