DO_NOT_MERGE: Try to avoid pointers in repeated fields #2002
Conversation
Signed-off-by: Bogdan Drutu <[email protected]>
In this experiment, the repeated InstrumentationLibrarySpans field is marked as not nullable: * There is one important breaking change, after Append is called, changing the object that was used to call Append will not reflect into changes in the appended object (essentially we do a copy). * IsNil will always return false. * We can easily and optimal provide now a constructor with capacity, instead of New + Resize. * Calling Append multiple times is more innefficient than Resize/fill object. Need to copy the entire object. Signed-off-by: Bogdan Drutu <[email protected]>
| @@ -223,17 +223,17 @@ func (ms ResourceSpans) CopyTo(dest ResourceSpans) { | |||
| type InstrumentationLibrarySpansSlice struct { | |||
| // orig points to the slice otlptrace.InstrumentationLibrarySpans field contained somewhere else. | |||
| // We use pointer-to-slice to be able to modify it in functions like Resize. | |||
| orig *[]*otlptrace.InstrumentationLibrarySpans | |||
| orig *[]otlptrace.InstrumentationLibrarySpans | |||
There was a problem hiding this comment.
how are you causing changes in this generated file without changes in pdatagen/ ?
There was a problem hiding this comment.
Manually changed to prove that I can pass tests and see how would it look like. As the description says I am not planning to merge this, just as a way to show how this can be done
|
fwiw, we also use non-pointers in slices in Jaeger, by configuring gogoproto generation. |
|
Yes, I think eventually we want this, but it would be useful to benchmark to see how much we gain. I think to gain the most we should start with the inner-most objects that are most numerous, e.g. KeyValue pointer slice in the AttributeMap or the StringKeyValue pointer slice in StringMap. |
|
@tigrannajaryan this is just to show how we can fix the panic issue, and how can we do this for one of the message. But it seems that there is an agreement that this is useful and will indeed start from bottom up. |
|
Just a notice: nil spans also cause panic. @bogdandrutu Maybe it's worth renaming the issue? I think this panic is not really critical now |
|
@Vemmy124 I know, any nil in any of the slices will cause that. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Signed-off-by: Tonis Tiigi <[email protected]> Co-authored-by: Tyler Yahn <[email protected]>
In this experiment, the repeated InstrumentationLibrarySpans field is marked as not nullable:
IMPORTANT this PR manually changes generated files just to prove things can be changed