size icon indicating copy to clipboard operation
size copied to clipboard

Does not take into account slice overhead with

Open csilvers opened this issue 3 years ago • 4 comments

If you have a struct

struct { a, b []int }

and you set mystruct.a = []int{1,2,3, ...., 100000} and mystruct.b = mystruct.a, then the memory taken by the slice should only be counted once. The existing code already does that correctly. However, the memory taken by the slice header should be counted twice, since both a and b have their own header. The existing code doesn't do this, and is thus off by 24 bytes for the above case.

Sadly, it's more complicated than that. If you have:

struct { a, b []int; c *[]int }

and do mystruct.a = ...; mystruct.b = mystruct.a; mystruct.c = &mystruct.a, then again the slice header should only be counted twice (once for a and once for b), plus an extra pointer for c. But the naive way to fix the first problem causes this code to be wrong, and to add struct header-size when calculating the size of c as well. This is because reflect.Pointer(slice) returns a pointer to sliceheader.Data, rather than to the whole sliceheader. But c points to the whole sliceheader. So to this module, it looks like c is pointing to a "new" slice, rather than one whose header-size has already been accounted for.

Probably the way to solve this is to change the pointer-handling code to say:

cache[v.Pointer()] = true
cache[v.Elem().Pointer()] = true

in the cases where v.Elem() has a Pointer() defined (slice and map, though I think the above issues don't apply for map).

csilvers avatar Oct 28 '21 22:10 csilvers

Hm, I think I'm confused as to what the problem actually is here. The issue I found is when I tried to fix the first issue by changing the slice case to:

		if v.Kind() != reflect.Array && cache[v.Pointer()] {
			return int(v.Type().Size())
		}

(instead of return 0), then v8 failed because it was double-counting something. But I'm not sure what.

csilvers avatar Oct 28 '21 22:10 csilvers

Oh wait, I think that is the actual problem, but the solution is backwards. We want to change the Ptr case to do:

		if cache[v.Pointer()] || cache[v.Elem().Pointer()] {
			return int(v.Type().Size())
		}

(but only in the case v.Elem() can be pointer-ified.)

csilvers avatar Oct 28 '21 22:10 csilvers

Ugh, that solution doesn't work either. My head hurts thinking about what the right way to do this is.

csilvers avatar Oct 28 '21 23:10 csilvers

Probably Go doesn't have built-in generic Size() func for a reason :) I haven't used Reflect package for years and following your thoughts hurts me also. I don't have solution for problem you specified and unfortunately don't have time for research.

DmitriyVTitov avatar Oct 28 '21 23:10 DmitriyVTitov