Skip to content
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

Put operation should return a shared_ptr to the object, otherwise a copy is always needed #35

Open
alexeysofin opened this issue Jan 15, 2025 · 2 comments · May be fixed by #36
Open

Put operation should return a shared_ptr to the object, otherwise a copy is always needed #35

alexeysofin opened this issue Jan 15, 2025 · 2 comments · May be fixed by #36

Comments

@alexeysofin
Copy link

Almost always cache check/put operations are followed by returning results to the callee, e.g.

auto val = std::shared_ptr{nullptr};

const auto [cached_val, cached_ok] = cache.TryGet(key);

if (!cached_ok) {
  # for example, val is already a shared_ptr
  val = Calc(key);
  cache.Put(key, val);
  return val;
}

return cached_val;

so current implentation doesn't allow storing shared_ptr, only shared_ptr<shared_ptr>.

Otherwise we must always copy the object to the cache because Put doesn't return a shared_ptr for newly created smart pointer, if it did, we could have done

const auto [cached_val, cached_ok] = cache.TryGet(key);

auto val = SomeType{};

if (!cached_ok) {
  val = Calc(key);
  auto val_shared_ptr = cache.Put(key, std::move(val));
  return val_shared_ptr;
}

return cached_val;

although, looking again at the code, even with a move in the latter case, Insert will anyways initiate a copy constructor, which does not seem right for large objects

@vpetrigo
Copy link
Owner

Hello @alexeysofin,

Probably, I am not completely following the issue you are trying to solve. The default cache type is shared_ptr<T>. So, introducing a void Put(const Key &key, Value &&value) noexcept should resolve the issue with unnecessary copying. Something like that:

void Put(const Key &key, Value &&value) noexcept
{
    // ...
    if (elem_it == cache_items_map.end())
    {
        // ...
        Insert(key, std::forward(value));
    }
    // ...
}

void Insert(const Key &key, Value &&value)
{
    cache_items_map.emplace(std::make_pair(key, std::make_shared<Value>(std::forward<Value>(value))));
}

Regarding returning a newly added value with Put: you can use the Get for retrieving a value by key which returns a shared_ptr to a value. 🤔

@alexeysofin
Copy link
Author

hi @vpetrigo

introducing a void Put(const Key &key, Value &&value) noexcept

that's what I've done in essence in the PR, your option without a template and a second method is fine too as well, I guess.

you can use the Get for retrieving a value by key which returns a shared_ptr to a value

what if we're inside a concurrent environment? It can happen that another thread evicts this key, and will get a cache miss. Another reason is performance, why do a second Get for no reason?

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 a pull request may close this issue.

2 participants