-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add filter to HashSet/Table/Map #588
base: master
Are you sure you want to change the base?
Conversation
55b2338
to
d0741d2
Compare
With the removal of the raw table, it is hard to implement an efficient loop to conditionally remove/keep certain fields up to a limit. i.e. a loop that can be aborted and does not require rehash the key for removal of the entry.
not sure if the name makes sense... maybe |
You can already do this with let mut removed = 0;
for _ in map.extract_if(|&k, _| k % 2 != 0) {
removed += 1;
if removed == 3 {
break;
}
} |
Thanks @cuviper , does this still loop over all hash entries tho? If the intention is to loop over all entries, then the exisiting retain works as well, as one's predicate can just stop returning false, once it returned enough trues. |
Nvm, I see how it works now, it certainly did not catch my attention how it works :), thought it works as retain, maybe I'll add an example to the documentaion of extract_if to capture this use case. |
I suppose a limitation compared to your proposal is that you can only stop |
Yeah, I was trying to write a test with my use case, and found it awkward if I would like to break out early before the next matched entry. |
@cuviper do you think then the addition of the API still serves a purpose, the main use case for the new api is to limit the # of iterations per access to the hashmap, and extract_if certainly does not serve that purpose well as discussed. |
As a workaround, you could extract that final item to terminate the loop, and then re-insert it. Even with rehashing, that may be faster than continuing to iterate the rest of the map. Otherwise, yes I do think this API can serve a unique purpose, but If we do add something -- it would be slightly more powerful returning |
Yes, that's what I'm trying to write a test for, and I really find it awkward, it ends up like this.
I did debate whether Option is the right approach, just wanted something lite weight to pass the stop signal. But yes, I can see it can be more powerful with an explicit flow control. I can work towards this if that's something you guys would more likely to accept. |
Removing the raw table complicates implementing efficient loops that conditionally remove or keep elements and control iteration flow, particularly when aborting iteration without rehashing keys for removal. The existing `extract_if` method is cumbersome for flow control, limiting flexibility. The proposed addition addresses these shortcomings, by enabling proper flow control to allow efficient iteration and removal of filtered elements.
Now renamed the function to |
@cuviper any feedback with the current PR? As mentioned, I'll work towards any suggestion that would improve this use case. |
I don't love the name |
Yeah, same, I'd be open to any suggestions with the naming. I couldn't come up with anything better, my candidates are, |
Can't you implement the same functionality by just using a |
Thank you @Amanieu! I just tried it again,
|
That print is an observable side effect -- the compiler cannot skip that by optimization. |
@cuviper After rerun I found even with the simplest test, it is still failing (so my previous comment wasn't correct):
|
I guess my point is, if one has to craft their code so carefully to make sure the loop ends when it should, it signals the need for this more explicit API, no?
|
The values of (That doesn't include explicit debug/release differences like debug-assertions and overflow-checks, which change the code that is produced even before optimization starts.) If you want the optimizer to see that it can skip the rest of the loop, you should "break" in a way that doesn't modify anything that would be visible outside that loop.
Maybe, if that use case is deemed significant enough to warrant a larger API. I'm sure there are a lot of things we can imagine that would be easier to accomplish with some specialized API, but it doesn't mean we should add them all. |
@cuviper thank you for your thoughtful comments. I understand your perspective, but I respectfully disagree that users of this library need to have in-depth knowledge of compiler internals to optimize their code. In my experience as shown above, achieving optimal performance can be quite challenging.
I feel this was misunderstood. My intention was not to propose an API that's completely new or unreasonable, but rather to share a specific use case that might not have been considered seriously previously, and it was achievable through the raw table apis that are now taken away. If removing the raw table API is in favor of the use of the hashbrown APIs (which is a good direction IMO), then I felt there is a gap that should be filled by this extra API. I think I tried to provide realistic and relevant examples, and I'm open to reasonable workarounds that address the concerns. |
I'm also not trying to dismiss your case, just offering a reason why we might not address it. Sometimes the answer can be "no" for other reasons too, like maintenance burden. Either way, this PR is not decided yet! |
Just to finish the topic about the compiler optimization:
This is the assembly from the
I'm not great at assembly code, but afaik, this does not early exit the loop afaik. Use GDB to log the count of the @cuviper do you have more things to try? I could give it a couple of more iterations just to close off this topic. |
I did feel a bit discouraged as you could see. But again, I would be convinced if any reasonable workarounds are available to normal users like myself, or any suggestions to make this API more acceptable to be included. Having lost the access to the raw table already incurred extra maintenance to the users of the library afaik, but I support that as it would reduce the maintainence cost to this great library. |
There's also the fun fact that |
True, I mean, we could keep trying, but I feel like this is probably beyond any reasonable effort from a normal user :) |
Overall, I think this ties into the discussion we had a few days ago in the libs-api meeting about |
The cursor API looks interesting and promising for the general use cases, however implementation seems rather complex for what this api tries to fix, that is a simple flow control when looping over the elements. I'd say that's overkill, and performance wise I'm not sure that's going to be great comparing to a simple loop with a break. |
@Amanieu if you decide against this api after all, could you please give some directions to ppl like me who is basically stuck without the raw table access? I'm against forking unless absolutely necessary, yet I don't see a way forward as the use case is simply removed from the current offerings without much lead time. Even with the cursor API (which could be a long way to go), I'm not sure we will get the same performance. IMO, simplicity is sometimes better than a more general API that seemingly does everything. |
In the absence of a complete cursor API, I think your best bet would be to use |
but that requires creating at least an atomic variable, and have to insert the last element back, which seems quite hacky... do you see a way to avoid that?
|
You can use a |
Yes, true, I can profile it to see... but my guess is it is going to suffer, I'll post the results. |
For everyone's interest:
extract_if_vs_retain_with_control.rs:
results:
I'm not sure if things are optimized out in the |
You can also avoid re-inserting the last item if you return |
I'm not sure that's technically the matching use case anymore. Returning false with |
I fixed the test, it was flawed.
And yes, this seems to show insignificant performance gain with filter in this test case. The use of
|
Ironically, I ended up writing a trait for
|
With the removal of the raw table, it is hard to implement an efficient loop to conditionally remove/keep certain fields up to a limit. i.e. a loop that can be aborted and does not require rehash the key for removal of the entry.