Some modify and fix in ObjectCacheV2 #531
Conversation
common/objectcachev2.h
Outdated
| photon::thread* reclaimer = nullptr; | ||
| photon::common::RingChannel<LockfreeMPMCRingQueue<std::shared_ptr<V>, 4096>> | ||
| reclaim_queue; | ||
| photon::spinlock maplock; | ||
| std::unordered_set<Box, ItemHash, ItemEqual> map; |
There was a problem hiding this comment.
use box/Box instead of item/Item
| return *item; | ||
| } | ||
|
|
||
| uint64_t __expire() { | ||
| intrusive_list<Box> delete_list; | ||
| uint64_t now = photon::now; |
There was a problem hiding this comment.
It's simpler to use now - lifespan
There was a problem hiding this comment.
What if lifespan has been set a extra-big number (like UINT64_MAX, present object in objcache never reclaim
sat_add is necessary.
common/objectcachev2.h
Outdated
| auto x = lru_list.front(); | ||
| while (x && (photon::sat_add(x->timestamp, lifespan) < now)) { | ||
| lru_list.pop(x); | ||
| __update(*x, nullptr); |
There was a problem hiding this comment.
it's more performant to realize x->reset(), which simply invokes shared_ptr::reset().
There was a problem hiding this comment.
In new implementation still use exchange/swap like stuff instead of reset().
swapped out object still have at lease one reference if result is assigned to another variable, so it could able to control when to perform object destruction.
common/objectcachev2.h
Outdated
| _reader(std::move(reader)), | ||
| _recycle(false) {} | ||
| Borrow(ObjectCacheV2* oc, Box* item, const std::shared_ptr<V>& reader) | ||
| : _oc(oc), _item(item), _reader(reader), _recycle(false) {} | ||
|
|
||
| Borrow(Borrow&& rhs) : _reader(nullptr) { *this = std::move(rhs); } | ||
|
|
||
| Borrow& operator=(Borrow&& rhs) { |
There was a problem hiding this comment.
Instead of swap/exchange, it's more semantically right to:
assert(_oc == rhs._oc);
std::atomic_exchange(&_reader, rhs._reader);
_item = rhs._item;
_recycle = rhs._recycle;
rhs._reader.reset();
rhs._item = nullptr;
rhs._recycle = false;There was a problem hiding this comment.
according to cppreference https://en.cppreference.com/w/cpp/memory/shared_ptr
If multiple threads of execution access the same shared_ptr object without synchronization and any of those accesses uses a non-const member function of shared_ptr then a data race will occur
Since atomic<shared_ptr> is not supported by c++11/14/17, an new implementation using photon::mutex helps keep shared_ptr update in critical zone.
| return *item; | ||
| } | ||
|
|
||
| uint64_t __expire() { | ||
| intrusive_list<Box> delete_list; | ||
| uint64_t now = photon::now; |
common/objectcachev2.h
Outdated
| reclaim_queue.template send<PhotonPause>(nullptr); | ||
| photon::thread_join((photon::join_handle*)reclaimer); | ||
| reclaimer = nullptr; | ||
| } | ||
| SCOPED_LOCK(maplock); |
There was a problem hiding this comment.
use maplock only to protect a single assignment?
There was a problem hiding this comment.
code here want to access intrusive_list only for preventing hit assert of list.
Technically in dtor, other threads should not have any other accesses to the whole object cache, the lock here could be ignored.
I will remove the lock acquiring here
| photon::now) { | ||
| r = __update(item, ctor()); | ||
| auto r = std::shared_ptr<V>(ctor()); |
There was a problem hiding this comment.
ctor() may return a shared_ptr?
common/objectcachev2.h
Outdated
| Box& __find_or_create_box(KeyType&& key) { | ||
| Box keybox(key); | ||
| Box* box = nullptr; | ||
| { |
common/objectcachev2.h
Outdated
| std::swap(r, ref); | ||
| return r; | ||
| } | ||
| std::shared_ptr<V> clear(uint64_t ts = 0) { |
| // make vector holds those shared_ptr | ||
| // prevent object destroy in critical zone | ||
| to_release.push_back(x->clear()); | ||
| map.erase(*x); |
There was a problem hiding this comment.
use map.extract().value() obtain the Box element, so as to use the intrusive_list for to_release.
There was a problem hiding this comment.
Seems add macro to check c++ version to show different implementation for those above C++17 is a good idea.
There was a problem hiding this comment.
Seem that C++17 node_handle owns the element Box, can not get out the pointer to Box without hacking.
Some fixes.