-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make ServiceStats an intrinsic #5
make ServiceStats an intrinsic #5
Conversation
andreasgerstmayr
commented
Apr 23, 2024
- make ServiceStats an intrinsic
- skip allocating ServiceStats in rebatchIterator if already allocated
- clone ServiceStats in Spanset.clone()
* make ServiceStats an intrinsic * skip allocating ServiceStats in rebatchIterator if already allocated * clone ServiceStats in Spanset.clone() Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
pkg/traceql/storage.go
Outdated
@@ -175,6 +172,7 @@ func (s *Spanset) Release() { | |||
|
|||
func (s *Spanset) clone() *Spanset { | |||
ss := *s | |||
ss.ServiceStats = maps.Clone(s.ServiceStats) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not 100% sure if a deep clone is required here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SpanSet also contains the other data structures like Attributes []*SpansetAttribute
and Spans []Span
. Both are passed to ss
without creating a deep clone first. I think it would be more consistent to do the same for ServiceStats
.
I also checked all the parts where s.clone()
is called and it does not look like a deep clone is required here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking! I removed the maps.Clone().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment about the deep clone
Otherwise the PR looks good to me 👍
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>