perf: Port FrozenOrderedSet to rust#23200
Conversation
42a564e to
ca376c9
Compare
ee17c67 to
0bf4cba
Compare
|
Really wish I knew rust better for all these cool performance cases. Cross referencing: #14719 |
0bf4cba to
c52ba37
Compare
|
What about just porting |
I assume because |
I dont know. It is used a lot. |
cburroughs
left a comment
There was a problem hiding this comment.
The __new__ --> __init__ thing needs a oneline mention in the release notes I think, on the off chance someone was using the class?
I ran it through some local benchmarks which suggest single digit percentage performance improvements. I think that matches your expectations? As idle curiosity, I wonder how much of the benefit comes from the lazy hash computation.
| .map(|o| to_pyset(&o)) | ||
| .collect::<PyResult<Vec<_>>>()?; | ||
| filter_keys(self, py, |key| { | ||
| Ok(sets.iter().all(|s| s.contains(key).unwrap_or(false))) |
There was a problem hiding this comment.
rust clarity: Is the unwrap_or here intentional? I'm not sure why intersection should handle this type of error differently than difference?
There was a problem hiding this comment.
Not intentional. This would set.__contains__ throwing an exception. Not sure that ever happens, but better to fix it and let the weird case be visible at least 🤷♂️
c52ba37 to
5b7d8f5
Compare
5b7d8f5 to
5bdcfaf
Compare
Aye, around 1%, it scales well with large repos with many deps. The python impl also enjoyed a lazy hash, so the port should be net-nothing if it is never accessed. It almost always is though... I've updated release notes and fixed the correctness bug you caught |
Followup to #22501. Same approach —
FrozenOrderedSetis now a pyo3#[pyclass]backed byPy<PyDict>with lazy hash viaOnceLock. The end goal is porting more rule code to rust intrinsics.