Skip to content

Conversation

@manugoyal
Copy link
Contributor

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
Copy link
Contributor

milianw commented Jun 28, 2022

@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
Copy link
Contributor

milianw commented Jun 28, 2022

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
Copy link
Contributor

milianw commented Jun 28, 2022

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!

manugoyal added 2 commits July 3, 2022 16:22
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.
@manugoyal manugoyal force-pushed the manu/augment-uprase-fn-signature branch from 61a8c1b to a44cfdb Compare July 4, 2022 00:23
@manugoyal manugoyal merged commit 735a74b into master Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants