Implement size estimation function for structs used in history event cache #6714
Conversation
…mpty size estimation function for mutableState and shardContext
| size += 8 | ||
| } | ||
|
|
||
| if v.RetryPolicy != nil { |
There was a problem hiding this comment.
RetryPolicy can also implement Size(). It shouldn't be hardcoded to 24. It has a string list field
| if v.RetryPolicy != nil { | ||
| size += 24 | ||
| } | ||
| if v.Header != nil { |
There was a problem hiding this comment.
Same here. Header size is not fixed to 16. Let's calculate it on the fly based on map content
| size += 24 | ||
| if v.WorkflowExecution.WorkflowID != "" { | ||
| size += uint64(16 + len(v.WorkflowExecution.WorkflowID)) | ||
| } | ||
| if v.WorkflowExecution.RunID != "" { | ||
| size += uint64(16 + len(v.WorkflowExecution.RunID)) | ||
| } |
There was a problem hiding this comment.
you are doing this calculation on WorkflowExecution type field in multiple places. Just expose Size() on that type and call it.
There was a problem hiding this comment.
let's make sure we avoid redundant calculations like this for other typed fields
| if v.Cause != nil { | ||
| size += 8 | ||
| } |
There was a problem hiding this comment.
you can have a util function which checks type of the passed argument and returns size if it's a known type.
e.g.
func SizeEstimate(in any) uint64 {
switch in.(type) {
case int64: return 8, true
case string: return 16 + len(in)
...
}
return 0
}
There was a problem hiding this comment.
this can eliminate all the hardcoded numbers spread all over. also simplifies the caller side by eliminating != nil checks. you can do that check in the helper function
There was a problem hiding this comment.
I suspect the unsafe.Sizeof() package (credit to Ignat for pointing that out) might help some calcualtions here.
| @@ -3060,6 +3684,185 @@ func (v *HistoryEvent) GetUpsertWorkflowSearchAttributesEventAttributes() (o *Up | |||
| return | |||
| } | |||
|
|
|||
| // Size is an internal method to get the estimated size of the event | |||
| func (v *HistoryEvent) Size() uint64 { | |||
There was a problem hiding this comment.
We should somehow make this future proof so when someone introduces a new attribute the corresponding unit test fails because of size mismatch.
| @@ -63,6 +63,21 @@ func (v *ActivityTaskCancelRequestedEventAttributes) GetActivityID() (o string) | |||
| return | |||
| } | |||
|
|
|||
| // Size returns the approximate memory used in bytes | |||
| func (v *ActivityTaskCancelRequestedEventAttributes) Size() uint64 { | |||
| if v == nil { | |||
There was a problem hiding this comment.
A question: Can we generate this code (with gowrap or something else) for all the structs instead of writing it manually?
Still in drafting mode due to git commit is combined with another PR, main change is in the shared.go
What changed?
Implement Size() for structs referred in history event cache
Why?
We want to modernize existing cadence common cache implement to a bytes-based system. That means we need to have a method to measure each entry (which is currently accepting any generic interface). We found the "Reflect" package provides a measuring function but runtime is too slow to be used in cache operations. Therefore, we will require all usages to implement the Size() function in their cache logic if they want to migrate to the new bytes-based system.
In order to seamless transition from the current cache system with an entry-based model, the implementation and rollout will be done in following phases:
Define the Sizeable interface
Implement Sizeable for cadence-history service
a. Implement Size() for ExecutionCache
b. Implement Size() for EventCache <-- This PR
Implement bytes-based cache system
Enable new cache system for usage in cadence-history service
Implement and enable new cache system for the remaining usages
How did you test it?
Unit tests
Potential risks
No risk since this PR only adds read-only function that is not used.
Release notes
Documentation Changes