Consistently use int64 type for string refs in pprofextended. #560
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -235,7 +235,7 @@ message Sample { | |
| // Supersedes location_index. | ||
| uint64 locations_length = 8; | ||
| // A 128bit id that uniquely identifies this stacktrace, globally. Index into string table. [optional] | ||
| int64 stacktrace_id_index = 9; | ||
|
There was a problem hiding this comment. This field got removed with #575. Maybe we can close the PR now? |
||
| // The type and unit of each value is defined by the corresponding | ||
| // entry in Profile.sample_type. All samples must have the same | ||
| // number of values, the same as the length of Profile.sample_type. | ||
|
|
@@ -355,7 +355,7 @@ message Location { | |
| bool is_folded = 5; | ||
|
|
||
| // Type of frame (e.g. kernel, native, python, hotspot, php). Index into string table. | ||
| int64 type_index = 6; | ||
|
There was a problem hiding this comment. As #557 got merged, this should no longer be relevant. |
||
|
|
||
| // References to attributes in Profile.attribute_table. [optional] | ||
| repeated uint64 attributes = 7; | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Semi off-topic, but was wondering if it would make sense to change this field. I feel that the space savings of referencing the string table might not be that big and it's not clear what encoding this would have in the string table (base64? ascii? would the string have to be unicode? etc):
uint64, storing the high and low bits for the stack ID;bytesfield;Profilemessage;curious on your thoughts here! cc @florianl
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.
Using bytes or string would mean another indirection / dynamic allocation for the in-memory representation so I would be careful with that.
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.
That's a good point. I personally prefer having this as two
uint64s but not sure if everyone would agree with the slightly increase in memory. I am not a protobuf expert, I am assuming that adding the extrauint64field would increase the size of the message by 10 Bytes if I am understanding the docs rightThere 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 concept of a
stacktrace_idis new and google/pprof doesn't know this element. It originates from the original optimyze stateful protocol and helped two communicate how often a trace was seen, while not sending the full stack trace every time. Back then, we went with 128bit as we wanted the stacktrace IDs to be globally unique and reduce collisions. I'm not sure how this field is used by other implementations of the protocol. So havingstacktrace_id_indexas a index into the string table is the most flexible way for the moment, I think.It is just the type of the filed, that needs better alignment. For every index
int64is used (also in google/pprof) and so the current type ofuint32should change and align.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.
Makes sense, in that case, should we update the comment and leave the number of bits for the stack id unspecified?
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 sure how relevant this PR is anymore, after the discussion around the google/pprof donation in the last meeting. There are some cases where the profiling protocol can and should evolve and maybe this is one of them.