On 19/11/2019 12:48, Kyotaro Horiguchi wrote:
> 1. Inserting a branch in SearchCatCacheInternal. (CatCache_Pattern_1.patch)
>
> This is the most straightforward way to add an alternative feature.
>
> pattern 1 | 8459.73 | 28.15 # 9% (>> 1%) slower than 7757.58
> pattern 1 | 8504.83 | 55.61
> pattern 1 | 8541.81 | 41.56
> pattern 1 | 8552.20 | 27.99
> master | 7757.58 | 22.65
> master | 7801.32 | 20.64
> master | 7839.57 | 25.28
> master | 7925.30 | 38.84
>
> It's so slow that it cannot be used.
This is very surprising. A branch that's never taken ought to be
predicted by the CPU's branch-predictor, and be very cheap.
Do we actually need a branch there? If I understand correctly, the point
is to bump up a usage counter on the catcache entry. You could increment
the counter unconditionally, even if the feature is not used, and avoid
the branch that way.
Another thought is to bump up the usage counter in ReleaseCatCache(),
and only when the refcount reaches zero. That might be somewhat cheaper,
if it's a common pattern to acquire additional leases on an entry that's
already referenced.
Yet another thought is to replace 'refcount' with an 'acquirecount' and
'releasecount'. In SearchCatCacheInternal(), increment acquirecount, and
in ReleaseCatCache, increment releasecount. When they are equal, the
entry is not in use. Now you have a counter that gets incremented on
every access, with the same number of CPU instructions in the hot paths
as we have today.
Or maybe there are some other ways we could micro-optimize
SearchCatCacheInternal(), to buy back the slowdown that this feature
would add? For example, you could remove the "if (cl->dead) continue;"
check, if dead entries were kept out of the hash buckets. Or maybe the
catctup struct could be made slightly smaller somehow, so that it would
fit more comfortably in a single cache line.
My point is that I don't think we want to complicate the code much for
this. All the indirection stuff seems over-engineered for this. Let's
find a way to keep it simple.
- Heikki
