Re: Protect syscache from bloating with negative cache entries

At Tue, 17 Nov 2020 17:46:25 +0200, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote in
> On 09/11/2020 11:34, Kyotaro Horiguchi wrote:
> > At Fri, 6 Nov 2020 10:42:15 +0200, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
> > wrote in
> >> Do you need the "ntaccess == 2" test? You could always increment the
> >> counter, and in the code that uses ntaccess to decide what to evict,
> >> treat all values >= 2 the same.
> >>
> >> Need to handle integer overflow somehow. Or maybe not: integer
> >> overflow is so infrequent that even if a hot syscache entry gets
> >> evicted prematurely because its ntaccess count wrapped around to 0, it
> >> will happen so rarely that it won't make any difference in practice.
> > That relaxing simplifies the code significantly, but a significant
> > degradation by about 5% still exists.
> > (SearchCatCacheInternal())
> > + ct->naccess++;
> > !+ ct->lastaccess = catcacheclock;
> > If I removed the second line above, the degradation disappears
> > (-0.7%).
>
> 0.7% degradation is probably acceptable.

Sorry for the confusion "-0.7% degradation" meant "+0.7% gain".

> > However, I don't find the corresponding numbers in the output
> > of perf. The sum of the numbers for the removed instructions is (0.02
> > + 0.28 = 0.3%). I don't think the degradation as the whole doesn't
> > always reflect to the instruction level profiling, but I'm stuck here,
> > anyway.
>
> Hmm. Some kind of cache miss effect, perhaps? offsetof(CatCTup, tuple) is

Shouldn't it be seen in the perf result?

> exactly 64 bytes currently, so any fields that you add after 'tuple' will go
> on a different cache line. Maybe it would help if you just move the new fields
> before 'tuple'.
>
> Making CatCTup smaller might help. Some ideas/observations:
>
> - The 'ct_magic' field is only used for assertion checks. Could remove it.

Ok, removed.

> - 4 Datums (32 bytes) are allocated for the keys, even though most catcaches
> - have fewer key columns.
> - In the current syscaches, keys[2] and keys[3] are only used to store 32-bit
> - oids or some other smaller fields. Allocating a full 64-bit Datum for them
> - wastes memory.

It seems to be the last resort.

> - You could move the dead flag at the end of the struct or remove it
> - altogether, with the change I mentioned earlier to not keep dead items in
> - the buckets

This seems most promising so I did this. One annoyance is we need to
know whether a catcache tuple is invalidated or not to judge whether
to remove it. I used CatCtop.cache_elem.prev to signal the same in
the next version.

> - You could steal a few bit for dead/negative flags from some other field. Use
> - special values for tuple.t_len for them or something.

I stealed the MSB of refcount as negative, but the bit masking
operations seems making the function slower. The benchmark-2 gets
slower by around +2% as the total.

> With some of these tricks, you could shrink CatCTup so that the new lastaccess
> and naccess fields would fit in the same cacheline.
>
> That said, I think this is good enough performance-wise as it is. So if we
> want to improve performance in general, that can be a separate patch.

Removing CatCTup.dead increased the performance of catcache search
significantly, but catcache entry creation gets slower for uncertain
rasons..

(Continues to a reply to Robert's comment)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center