Conversation
There was a problem hiding this comment.
main changes in this file
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
coszio
left a comment
There was a problem hiding this comment.
I ran the benchmark locally after introducing a couple more black_boxes
It does have better performance
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
6943774 to
aab2245
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // AI says, that if atomic is not updated frequently, | ||
| // it is cheap enough to check it on every iteration. | ||
| if self.is_stopped.load(Ordering::Relaxed) { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
So here's a stupid idea:
I'm curious if we could just ignore atomics here and concurrently set the boolean.
It'd be cheaper by not using atomic operations, at the cost of potentially 'recognizing' the changed flag a bit later due to local caches.
There was a problem hiding this comment.
Turns out the non-atomic variant is exactly the same for reads on x86_64 and ARM:
https://rust.godbolt.org/z/oe4f17xEo
Thanks @xzfc !
| fn next(&mut self) -> Option<Self::Item> { | ||
| // AI says, that if atomic is not updated frequently, | ||
| // it is cheap enough to check it on every iteration. | ||
| if self.is_stopped.load(Ordering::Relaxed) { |
There was a problem hiding this comment.
I also checked the unlikely and cold_path intrinsic just for fun, but they aren't stable yet :(
|
Thanks for the benchmarks supporting the change 👍 |
* add stopflag check into query_points of payload index * propagate stopflag more * fmt * bench: compare better against no atomic * use stoppable iter with IteratorExt * clippy * Add cancelled error helper --------- Co-authored-by: Luis Cossío <luis.cossio@outlook.com> Co-authored-by: timvisee <tim@visee.me>
iter_filtered_pointsunder some special conditions could consume significant resources and didn't respect stopflag in all cases.Now it does.
I went ahead with another implementation of stopable iterator, because it is easier and apparently also slightly faster (i did benchmark).
I tried to remove newly introduced double checking where I spotted it.
Also we want to check stopflag right after starting search, cause it could have become outdated while waiting in runtime queue.