improve object cache expire efficiency #521
Conversation
common/expirecontainer.cpp
Outdated
| SCOPED_LOCK(_lock); | ||
| _list.split_by_predicate([&](Item* x) { | ||
| bool ret = x->_timeout.expiration() < photon::now; | ||
| if (ret) _set.erase(x); | ||
| return ret; | ||
| }); | ||
| }).delete_all(); | ||
| } | ||
| _list.delete_all(); | ||
| return 0; |
There was a problem hiding this comment.
I think the so called 'Statements and Declarations in Expressions' brings nothing but vagueness, and it's not c++ standard.
It implicitly relies on the fact that the object in the expression is a new different one. It may cause confusion to regular developers.
struct A {
void f () {
cout << this << endl;
}
};
int main() {
A a;
cout << &a << endl;
({a;}).f();
// Address not same
}
There was a problem hiding this comment.
It called "Compound Statement Expressions" or "Statement Expression", and it is a long history feature in almost every C/C++ compiler such as GCC/Clang/MSVC/Intel CC/IBM Compiler... It is a kind of "De Facto Standard"
Statement expression is widely used in a lot of famous projects like
Linux kernel
https://github.com/torvalds/linux/blob/5bbd9b249880dba032bffa002dd9cd12cd5af09c/drivers/most/core.c#L89
SPDK
https://github.com/spdk/spdk/blob/a44a9620a68f2a8c29db1b7165d67fe7c070458c/lib/env_ocf/ocf_env.h#L65
liburing
https://github.com/axboe/liburing/blob/7b3245583069bd481190c9da18f22e9fc8c3a805/src/lib.h#L32
It is hard to say "brings nothing but vagueness".
Since ({a;}) means a expression, the result have to be a pr-value (temporary object) with type A, it will be a new object differ from a.
A purely C++ way is using inline lambda evaluation instead of statement expression. The code in example should basically equivalent to [&]{return a;}().f();, or with less type deducing, [&]()->A {return a;}().f();
There was a problem hiding this comment.
Statement expression is a important in Photon ALog, and cannot be substitute by lambda.
Line 462 in 7c59780
common/expirecontainer.cpp
Outdated
| ({ | ||
| if (_lifespan > UINT64_MAX / 2) | ||
| return 0; | ||
| { |
There was a problem hiding this comment.
if timeout == -1UL, just pass.
| @@ -39,6 +39,7 @@ namespace photon | |||
| { | |||
| _on_timer = on_timer; | |||
| _default_timeout = default_timeout; | |||
| _reset_timeout = -1UL; | |||
There was a problem hiding this comment.
Line 1459 in 7c59780
There was a problem hiding this comment.
In timer stub, retry use _reset_timeout only when timer function returns a negative result. User should not interrupt timer with EAGAIN in any cases, but only call Timer::reset(new_timeout) method.
The _reset_timeout only be set when reset(new_timeout) is called. Set in constructor is meaningless.
common/expirecontainer.cpp
Outdated
| SCOPED_LOCK(_lock); | ||
| _list.split_by_predicate([&](Item* x) { | ||
| bool ret = x->_timeout.expiration() < photon::now; | ||
| if (ret) _set.erase(x); | ||
| return ret; | ||
| }); | ||
| }).delete_all(); | ||
| } | ||
| _list.delete_all(); | ||
| return 0; |
There was a problem hiding this comment.
It called "Compound Statement Expressions" or "Statement Expression", and it is a long history feature in almost every C/C++ compiler such as GCC/Clang/MSVC/Intel CC/IBM Compiler... It is a kind of "De Facto Standard"
Statement expression is widely used in a lot of famous projects like
Linux kernel
https://github.com/torvalds/linux/blob/5bbd9b249880dba032bffa002dd9cd12cd5af09c/drivers/most/core.c#L89
SPDK
https://github.com/spdk/spdk/blob/a44a9620a68f2a8c29db1b7165d67fe7c070458c/lib/env_ocf/ocf_env.h#L65
liburing
https://github.com/axboe/liburing/blob/7b3245583069bd481190c9da18f22e9fc8c3a805/src/lib.h#L32
It is hard to say "brings nothing but vagueness".
Since ({a;}) means a expression, the result have to be a pr-value (temporary object) with type A, it will be a new object differ from a.
A purely C++ way is using inline lambda evaluation instead of statement expression. The code in example should basically equivalent to [&]{return a;}().f();, or with less type deducing, [&]()->A {return a;}().f();
| @@ -39,6 +39,7 @@ namespace photon | |||
| { | |||
| _on_timer = on_timer; | |||
| _default_timeout = default_timeout; | |||
| _reset_timeout = -1UL; | |||
There was a problem hiding this comment.
In timer stub, retry use _reset_timeout only when timer function returns a negative result. User should not interrupt timer with EAGAIN in any cases, but only call Timer::reset(new_timeout) method.
The _reset_timeout only be set when reset(new_timeout) is called. Set in constructor is meaningless.
beef9999
force-pushed
the
beef9999/br-21
branch
2 times, most recently
from
16901e6 to
efa8a09
Compare
common/expirecontainer.cpp
Outdated
| SCOPED_LOCK(_lock); | ||
| _list.split_by_predicate([&](Item* x) { | ||
| auto tmp = _list.split_by_predicate([&](Item* x) { |
There was a problem hiding this comment.
_list.split_by_predicate(...) will produce a new intrusive_list object, but intrusive_list has no move constructor or move assignment. That makes both tmp and pr-value of method result point to same list.
That makes pr-value _list.split_by_predicate(...) not empty after assignment, and then as a temporary object, goes to destruct, meets the assertion about intrusive_list __node == nullptr.
There was a problem hiding this comment.
Shall we add move semantics to intrusive_list, or just clean its pointer?
No description provided.