Scale private oh#4364
Conversation
|
Out of curiosity, did you try to measure contention after #4073? It removes essentially all lock operations attributed to the |
nigoroll
force-pushed
the
scale_private_oh
branch
from
805fee4 to
36f19db
Compare
| #define PRIVATE_OH_EXP 7 | ||
| static struct objhead private_ohs[1 << PRIVATE_OH_EXP]; |
There was a problem hiding this comment.
Why not define a constant?
#define PRIVATE_OH_EXP 7
#define PRIVATE_OH_CNT (1 << PRIVATE_OH_EXP)
static struct objhead private_ohs[PRIVATE_OH_CNT];I understand the purpose of vcountof() but if it confuses our tooling, it's maybe not worth using.
There was a problem hiding this comment.
How does it confuse our tooling?
There was a problem hiding this comment.
This part of the pull request description, before you edited it to strike the second sentence out:
The drawback of the chosen solution is that flexelint thinks all objheads would need to be from the array, and hence reports out of bounds access all over the place.
Suppressing these for the whole source file is far from ideal, but I have not found a better option (yet).
There was a problem hiding this comment.
The relevant part is the other bit of the description:
The choice of an array of objheads rather than an array of objhead pointers is motivated by hsh_deref_objhead_unlock(), which behaves differently for private objects.
This has nothing to do with vcountof(), what misleads flexelint is the fact that, with the static array for private objheads, the "others" are on the heap with arbitrary addresses. The reasoning of the tool is here "hey, if there is a check for an upper bound, but then an else branch, that else branch will probably be out of bounds".
I think the improved more selective silencing of flexelint is acceptable now.
| // https://probablydance.com/2018/06/16/fibonacci-hashing-the-optimization-that-the-world-forgot-or-a-better-alternative-to-integer-modulo/ | ||
| static inline size_t | ||
| fib(uint64_t n, uint8_t bits) | ||
| { | ||
| const uint64_t gr = 11400714819323198485LLU; | ||
| uint64_t r; | ||
|
|
||
| r = n * gr; | ||
| r >>= (sizeof(gr) * 8) - bits; | ||
| assert(r < (size_t)1 << bits); | ||
| return ((size_t)r); | ||
| } |
There was a problem hiding this comment.
Why not take the first byte of the request hash? No modulo involved, possibly one & PRIVATE_OH_MASK if you want less than 8 bits worth of private objheads.
There was a problem hiding this comment.
My main driver is vcl_synth {}. As requests to the same resource have the same hash, using it would defeat the purpose of the patch and not solve the lock contention, just move it to a lock "per host+url" (actually hash, and actually "set of...").
which? |
nigoroll
force-pushed
the
scale_private_oh
branch
from
36f19db to
bbdd566
Compare
With a lot of objects on the private objhead, the mtx would become contended. We solve this contention by using an array of private objheads and spreading objects over the array elements using a fibonacci hash. The choice of an array of objheads rather than an array of objhead pointers is motivated by hsh_deref_objhead_unlock(), which behaves differently for private objects. An alternative to the address check would be to add a flag field to struct objhead, but that would require to grow the struct (really not a good option for such a central data structure which, in many cases, is required once per object) or reduce the size of the magic value to 16 bits. The drawback of the chosen solution is that flexelint thinks all objheads would need to be from the array, and hence reports out of bounds access all over the place. These warnings are suppressed somehow specifically (see comment inline).
nigoroll
force-pushed
the
scale_private_oh
branch
from
bbdd566 to
445a843
Compare
3 participants
With a lot of objects on the private objhead, the mtx would become contended.
We solve this contention by using an array of private objheads and spreading objects over the array elements using a fibonacci hash.
The choice of an array of objheads rather than an array of objhead pointers is motivated by hsh_deref_objhead_unlock(), which behaves differently for private objects. An alternative to the address check would be to add a flag field to struct objhead, but that would require to grow the struct (really not a good option for such a central data structure which, in many cases, is required once per object) or reduce the size of the magic value to 16 bits.
The drawback of the chosen solution is that flexelint thinks all objheads would need to be from the array, and hence reports out of bounds access all over the place.
Suppressing these for the whole source file is far from ideal, but I have not found a better option (yet).These warnings are suppressed somehow specifically (see comment inline).For performance see #4365 (comment) : Without #4365, the benefits are not particularly visible because other locks will contend