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

objcachev2 #522

Merged
merged 5 commits into from
Jul 22, 2024
Merged

objcachev2 #522

merged 5 commits into from
Jul 22, 2024

Conversation

Coldwings
Copy link
Collaborator

A simpler but effective object cache implementation.

ObjectCacheV2 shows better performance compare to ObjectCache, with several
improvements:

  1. Less lock in both read and write.
  2. Object destruction always goes in background photon thread called reclaimer.
  3. Self adjustable timeout for reclaimer. No needs to set cycler timer.
  4. New update API, able to immediately substitute objects.
  5. ObjectCacheV2 no longer support acquire/release API.

It should work as ObjectCache in code do not depends on acquire/release API.

// other reader will get new one after updated
std::shared_ptr<V> update(V* val, uint64_t ts = 0,
std::shared_ptr<V>* newptr = nullptr) {
auto r = std::shared_ptr<V>(val);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you manage to integrate shared_ptr into ctor, so that there's a chance to use make_shared to improve performance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here accept plain V* type pointer is for compatibility, the ctor in ObjectCache returns only simple plain pointer.

It seems good to have a new specialization accept ctor returns shared_ptr directly.

LockfreeMPMCRingQueue<std::shared_ptr<V>*, 4096>>
reclaim_queue;
photon::spinlock maplock;
std::unordered_set<Box*, ItemHash, ItemEqual> map;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to use unordered_set<Box, ItemHash, ItemEqual>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible, the code might be a little tricky but tested work.


std::shared_ptr<V> __update(Box* item, V* val) {
std::shared_ptr<V> ret;
reclaim_queue.template send<PhotonPause>(new std::shared_ptr<V>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split it into two statements.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new std::shared_ptr<V>(...) looks evil. Try to avoid it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lockfree queue accepts only POD types, a shared_ptr<T> object cannot sen via lockfree queue.

Maybe I can try to specialize a lockfree queue with shared_ptr<T> type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

~ObjectCacheV2() {
_timer.stop();
if (reclaimer) {
reclaim_queue.template send<PhotonPause>(nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to use the template keyword?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is necessary.

Calling template member function needs a template prefix. The send<P> member function has default template argument ThreadPause, but here has to call with argument PhotonPause, makes here have to be with a template prefix.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have following code in RPC's exmaple:

    ExampleServer()
        : skeleton(photon::rpc::new_skeleton()),
          server(photon::net::new_tcp_socket_server()) {
        skeleton->register_service<Testrun, Heartbeat, Echo, ReadBuffer,
                                   WriteBuffer>(this);
    }

We call the template member function, register_service<...> without a template keyword.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but those code in RPC is a part of normal class.

Here is in template define.

ignore that template prefix will leads to erros

error: expected primary-expression before ‘>’ token

in GCC and

Use 'template' keyword to treat 'send' as a dependent template name (fix available)clang(missing_dependent_template_keyword)

in clang

@@ -481,7 +481,8 @@ TEST_F(RpcTest, passive_shutdown) {

// The passive shutdown took 3 seconds, until client closed the connection
GTEST_ASSERT_GT(duration, 2900);
GTEST_ASSERT_LT(duration, 3200);
// Since GH macos arm CI is slow, we allow 3.5 secs
GTEST_ASSERT_LT(duration, 3500);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm moving it to release/0.7

Copy link
Collaborator Author

@Coldwings Coldwings Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@lihuiba lihuiba merged commit 2056005 into alibaba:main Jul 22, 2024
10 checks passed
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.

2 participants