Support aggregate size computations over list, maps, and structs - #1404
Support aggregate size computations over list, maps, and structs#1404TristonianJones wants to merge 1 commit into
Conversation
efe3a1b to
42e38b0
Compare
| return uint32(n) | ||
| } | ||
|
|
||
| func safeUint32FromBoxedInt(v Int) uint32 { |
There was a problem hiding this comment.
nit: do you want to clamp the negatives? (-1 will return MaxUint32)
There was a problem hiding this comment.
I figure if you pass in a negative it's not intentional, and I should flip it to max as there's probably a bug. Thoughts?
There was a problem hiding this comment.
I think we should at least make this consistent one way or the other (safeUint32FromInt above clamps negatives).
As a general utility -- I'd think clamping to 0 is the common behavior but returning max uint32 is just fine to signal fail-closed for incorrect cost computation.
| case Bytes: | ||
| return safeUint32FromInt(len(v)) | ||
| case traits.Sizer: | ||
| return safeUint32FromBoxedInt(v.Size().(Int)) |
There was a problem hiding this comment.
Do you want to check for int type first? (Sizer's Size() returns ref.Val).
There was a problem hiding this comment.
Size is always an Int. At some point I was worried about Unknown and Err types, but the function guards prevent these values from ever being extracted from objects now -- they didn't in the early days and by then it was too late to change the interface.
There was a problem hiding this comment.
I was actually wondering about custom structs implementing Sizer and downstreams accidentally returning Uint (this is pretty contrived, feel free to just ack)
| return total | ||
| } | ||
|
|
||
| func getReflectValueAggregateSize(s AggregateSizer, fieldVal reflect.Value) uint32 { |
There was a problem hiding this comment.
wdyt about adding a guard for cyclic structs? (In case if the host application passes one in)
There was a problem hiding this comment.
I think adding a budget and depth make a lot of sense. I have this drafted, to a degree, in a memory limit PR, but let me move it over here.
To support best cost tracking, introduce version-aware
AggregateSizemethodson list, map, and object types.
The implementations currently memoize aggregate size computations
for immutable objects. The aggregate size is capped at
uint32to minimizethe allocation overhead associated with the book-keeping fields on these
objects.
As far as possible, the allocation cost associated computing the size
list, maps, and structs has been reduced to a minimum. The purpose of
having different, or versioned,
AggregateSizeimplementations is to allowusers to tailor which kinds of costs they're willing to consider before giving
up and assigning a maximum size.