efficient / libcuckoo

A high-performance, concurrent hash table
Other
1.62k stars 275 forks source link

Augment functor interface for uprase_fn and upsert. #141

Closed manugoyal closed 2 years ago

manugoyal commented 2 years ago

It is often useful to be able to operate over a newly-inserted value in the map, just as it is to operate over an already-existing value.

To enable this functionality, we support an additional overload to the functor passed to uprase_fn or upsert, which implements the signature operator()(mapped_type&, UpsertContext), where UpsertContext is an enum describing whether the value was newly-inserted or already existed in the table.

To mantain backwards compatibility, if the user's functor only implements operator()(mapped_type&), we will only invoke it if the value already existed in the table.

milianw commented 2 years ago

@manugoyal sorry for the overly long delay, I finally ported our code base to this new API and tested it. It works well, thank you very much! I would appreciate if this could get merged.

Thanks again

milianw commented 2 years ago

Ah or wait a second please, I now see that our CI found some regressions. I have to double check whether that's me porting to the new API incorrectly or something else.

milianw commented 2 years ago

Ah, the patch here is fine. It was simply a gotcha of the new API compared to the one I suggested originally in #138.

Previously I had:

Value bar(Key key)
{
    static libcuckoo::cuckoohash_map<Key, Value> cache;
    return cache.find_or_insert_fn(key, [&]() { return createValue(key); });
}

Then I ported it to the API here wrongly:

Value bar(Key key)
{
    Value ret;
    static libcuckoo::cuckoohash_map<Key, Value> cache;
    return cache.upsert(key, [&](Value &value, libcuckoo::UpsertContext context) {
        switch (context) {
        case libcuckoo::UpsertContext::ALREADY_EXISTED:
            break;
        case libcuckoo::UpsertContext::NEWLY_INSERTED:
            value = createValue(key);
            break;
        }

        value = ret;
    });
    return ret;
}

Note how I accidentally wrote value = ret; instead of ret = value.

I still believe it would be nicer to have something like find_or_insert_fn to simplify this usecase. But otherwise this patch works a charm, thanks!