Possible issue with table size in traversetable

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Possible issue with table size in traversetable

Igor Ehrlich
Dear list,
I believe that there is a little issue in the garbage collector implementation that was there all along at least since 5.1. This issue (at least this occurrence) does not affect stability and should have rather little performance impact, but I think it's worth fixing anyway, once confirmed.

Please consider a table with present array part and absent hash part. If I understand correctly, in this case Table.lsizenode will be equal to zero and no Node vector will be allocated. In this scenario, the memory footprint of such table will be equal to

sizeof(Table) + sizeof(TValue)*h->sizearray

However, during the footprint computation in traversetable, another part is added, related to hash vector, which is

... + sizeof(Node) * cast(size_t, sizenode(h))

The thing is, even with lsizenode equal to 0, sizenode(t) will return

( twoto((t)->lsizenode) ) == ( 1 << ((t)->lsizenode) ) == ( 1 << 0 ) == 1

thus erroneously adding sizeof(Node) to the return value.

To my mind, it could've been fixed by re-implementing sizenode:

#define sizenode(t)     ( (t)->lsizenode ? twoto((t)->lsizenode) : 0 )

Please take a look.


--
Best regards,
Igor A. Ehrlich
Reply | Threaded
Open this post in threaded view
|

Re: Possible issue with table size in traversetable

Roberto Ierusalimschy
> I believe that there is a little issue in the garbage collector
> implementation that was there all along at least since 5.1. This issue (at
> least this occurrence) does not affect stability and should have rather
> little performance impact, but I think it's worth fixing anyway, once
> confirmed.
>
> [...]
>
> The thing is, even with lsizenode equal to 0, sizenode(t) will return
>
> ( twoto((t)->lsizenode) ) == ( 1 << ((t)->lsizenode) ) == ( 1 << 0 ) == 1
>
> thus erroneously adding sizeof(Node) to the return value.

You are right, thanks for the feedback.


> To my mind, it could've been fixed by re-implementing sizenode:
>
> #define sizenode(t)     ( (t)->lsizenode ? twoto((t)->lsizenode) : 0 )

This fix is not correct, because 'sizenode' is used in many other places
that assume this behavior; note that tables with no hash part share a
common hash part of size 1. When computing the memory used by the table,
we should check whether the table is using this shared dummy node (see
macro 'isdummy') to compute its memory use.

-- Roberto