state: avoid unnecessary allocation object copies

[pkazmierczak]

While investigating NMD-1105 (possible memory leak in the event stream), I noticed that when we copy allocations, we first call the Copy method which returns us a deep copy of the object, and then we remove the job information from it to save space. Afterwards, we sanitize the allocation (i.e., we remove the SignedIdentities data) and during that operation we perform another copy.

This changeset doesn't necessarily resolve NMD-1105, but should definitely lower the memory footprint of eventFromChange.

[pkazmierczak]

Did a bit more digging on this because I thought perhaps we could get rid of copying node and allocation objects altogether, but sadly we need copies to sanitize the sensitive fields. We can actually avoid copying allocations because the state will accept an alloc with no SignedIdentities, but it won't accept a node without a SecretID. (not copying allocations is of course risky too, if we ever upsert an alloc with the same ID more than once we will lose the SignedIdentities field)

Possible solution for reducing the memory footprint of eventFromChange is making event paylod fields configurable. If we could skip some of the maps.Clone calls, that'd be a more significant improvement.

[jrasell]

LGTM!

[tgross]

Rather than introducing a new method and the flags on the existing methods, could we just move the call to Sanitize() here? That's what the node events do.

[pkazmierczak]

right but sanitize copies the whole alloc. We'd ideally skip the Job field. I suppose we could alternatively pass a parameter to Sanitize?

[tgross]

Oh right. There's an existing Allocation.CopySkipJob() method. I suspect we can use that safely in the Sanitize method rather than Allocation.Copy()?

[pkazmierczak]

Can we? Allocation.Sanitize() is called in alloc and node RPC endpoints, to return the full allocation object. Wouldn't we want the Job field present there?

[tgross]

CopySkipJob() doesn't nil-out the job, it just doesn't do the deep copy. So in the alloc and node RPC endpoints, we can call Sanitize() and it's safe to keep the pointer to the original copy of the job (because we're not mutating the job). And then here we can call Sanitize and then nil-out the job field (which doesn't touch the original job, just removes the pointer to it).

[pkazmierczak]

changed in 0e1c678

[pkazmierczak]

followed up with c96c250 which removes no longer needed sanitize param to alloc.copyImpl and cf7137d which nils-out the job field for the event payload.

[tgross]

LGTM (don't forget the changelog)