[PATCH v8 1/2] malloc: add tcache support for large chunk caching

[Cupertino Miranda]

Hi Wilco,

On 02-06-2025 18:05, Wilco Dijkstra wrote:
> Hi Cupertino,
> 
>> if (size <= TCACHE_SMALL_SIZE)
>>      {
>>         size_t tc_idx = csize2tidx (bytes);
>>         if (tcache != NULL && tcache->entries[tc_idx] != NULL)
>>            return tag_new_usable (tcache_get (tc_idx));
>>       }
>> else if (size <= mp_.tcache_max_bytes && tcache != NULL)
>>      {
>>        size_t tc_idx = large_csize2tidx (bytes);
>>        void *victim = tcache_get_large (tc_idx, nb);
>>        if (victim != NULL)
>>           return tag_new_usable (victim);
>>      }
> 
>> Any reason why in your suggested code, you do not check for tcache !=
>> NULL as the first condition for both cases ?
> 
> tcache is a thread-local variable so requires quite a few instructions.
> So what I did was to first check if the size is small to avoid redundantly
> checking tcache for larger blocks. This is also why I placed the
> large_csize2tidx after the large size check.
There is no way around loading tcache and I would assume that OoO engine 
would properly hide any ordering latency. In X86 I could see in v8 that 
the tcache benchmark, in the 'simple 'execution was getting a 
performance degradation, but with the 'optimized' was actually getting 
similar execution times. I assumed that x86 OoO engine was responsible.
> 
>>> Likewise (while we could possibly add large tcache support here in the future, it's likely
>>> a bad idea to preload tcache with large blocks).
>> I am confused why this is actually wrong?
>>
>> This change is not about adding large blocks support, it was rather
>> about removing the need for tcache_bins struct field and make all
>> tcache_put calls being checked against tcache_max_bytes, making
>> tcache_bins not needed.
> 
> But that is not possible since tcache_max_bytes now includes larger sizes.
> 
>> Now I wonder is any of this locations could be reached with a larger
>> chunk then the supported by the original fixed sized bins (up to 1kb
>> chunks) ? Otherwise even the original check against mp_.tcache_bins is
>> not enough.
> 
> The existing checks work correctly, so you either need to leave the
> definition of tcache_bins unchanged or check both tcache_max_bytes
> and TCACHE_SMALL_SIZE.
To be clearer for the purpose I will rename tcache_bins to 
tcache_small_bins, in the next version.

Cheers,
Cupertino