Avoid rechecking for duplicates in IdOrdMap::from_iter_unique

[izuzak]

Hello, again! 👋 While I was reading about IdOrdMap::from_iter_unique and writing up #300, it occurred to me that the method might be doing extra work.

Specifically, it seems like we're checking for duplicates twice:

• the first check happens in map.entry(value.key()) and that tells us whether this is a duplicate.
• the second check happens in the Entry::Vacant branch (i.e. after we already determined that this is not a duplicate) when entry.insert_ref(value) is called. insert_ref calls insert_unique_impl which also checks for duplicates.

        for value in iter {
            match map.entry(value.key()) {      // <---- first check
                Entry::Occupied(entry) => {
                    // return error for duplicate
                }
                Entry::Vacant(entry) => {
                    entry.insert_ref(value);    // <---- second check nested inside
                }
            }
        }

So, if I understood things correctly, we're checking for duplicates twice for a single unique-key insert, and once should be enough.

This PR is an attempt at a minimal change to avoid that extra work: extract the insertion logic out of insert_unique_impl so that it can be reused. This new method, insert_known_unique_impl (naming suggestions always welcome 🙈), is private and assumes that it doesn't need to check for duplicates itself because that has been done by the callers.

This method is what we then call from both insert_unique_impl and from_iter_unique after those methods determined that the value's key is unique. And that then removes the extra check for duplicates in from_iter_unique.

Again, I tried to start with a minimal diff, and if the general idea makes sense and feels valuable -- happy to iterate on the implementation based on feedback!

[sunshowers]

Thanks! Would you be willing to extend the same fix to IdOrdMap::insert_overwrite and IdHashMap::insert_overwrite?

Also, while we're optimizing here, from_iter_unique can reserve capacity using the size hint as well.

[izuzak]

Thanks for the very quick feedback! 🙇

Would you be willing to extend the same fix to IdOrdMap::insert_overwrite and IdHashMap::insert_overwrite? Also, while we're optimizing here, from_iter_unique can reserve capacity using the size hint as well.

Sure sure, happy to take a crack at it -- pushed a few more commits.

A note on what I did for IdHashMap: I again introduced an insert_known_unique_impl method and then used that in insert_overwrite. However, I did not make insert_unique_impl call insert_known_unique_impl as we did in IdOrdMap. As far as I can tell, insert_unique_impl uses key_to_item.entry(...), which both checks for duplicates and gives us a prepared vacant hash-table entry (in the non-duplicate case). Changing insert_unique_impl to use the new insert_known_unique_impl would throw away that prepared entry, recompute the hash, and go through a separate reserve/prehashed insert path (which felt like doing extra work). So, the new insert_known_unique_impl method is only used for insert_overwrite's higher-level Entry::Vacant path since that checks that the key is unique but does not expose the lower-level vacant hash-table entry.

It's possible there's a better/cleaner solution here with a bit more refactoring (e.g. maybe switching IdHashMap::insert_overwrite to use the key_to_item.entry(...) approach as well? 🤔). And also totally possible I misunderstood something. In any case, I started with a smaller diff since I wasn't 100% confident. Happy to continue iterating if you have thoughts/feedback and also perfectly fine if you'd prefer pushing to the branch directly if you have a specific idea for a cleaner approach.

Unrelated to this, the usage of BTreeSet for checking duplicates in IdOrdMap::insert_unique_impl and IdHashMap::insert_unique_impl also caught my eye. I think using BTreeSet here might not be necessary since the duplicate check can find at most one conflicting item. So, an Option<ItemIndex> or a direct early return might be enough. However, using BTreeSet does make the code more similar to the implementations in BiHashMap and TriHashMap, so overall it's a bit more consistent.

[sunshowers]

I would instead add a (non-public) VacantEntry::insert_known_unique that inserts the value without recomputing the hash (instead grabbing it from the VacantEntry).

[izuzak]

Ah, I like that -- thank you! 👍 And now it actually does do the thing that's written on the tin in the comment -- it avoids both the extra duplicate check and recomputing the hash (which I reintroduced 🙈). Done in 3017f0a.

EDIT: just a small note: since the method is non-public and the only caller doesn't need any result, it seemed okay for now to not return anything from the method.

[sunshowers]

Thanks! I pushed a few tweaks.