reporter: Do not generate unique dummy mappings#598
Conversation
christos68k
changed the title
| stringSet.Add("") | ||
| funcSet.Add(funcInfo{nameIdx: stringSet.Add(""), fileNameIdx: stringSet.Add("")}) | ||
| mappingSet.Add(dummyFileID) | ||
| dic.MappingTable().AppendEmpty() |
There was a problem hiding this comment.
@dmathieu Is there a chance that we can change pprofile.NewProfiles() in a way that the dictionary tables are already set up appropriately with default values?
Because to generate valid Profiles, this is currently done at "thousand" places and instead should be done only at a single place.
| mappingSet.Add(dummyFileID) | ||
| dic.MappingTable().AppendEmpty() |
There was a problem hiding this comment.
Should we do the same for the link table right in this PR?
Because the .proto says
// link_table[0] must always be set to a zero value default link,
// so that _index fields can use 0 to indicate null/unset.
There was a problem hiding this comment.
There are no links being set in the current code so we shouldn't explicitly add a zero value in this case as the entire field is optional as per proto3.
There was a problem hiding this comment.
The documentation says "must always be", that's pretty clear to me.
Also there is validator code in pdatatest that validates if a given profile is valid or not. In the future, it will consider a message invalid if the default entry is missing.
Adding the default link value won't hurt, performance or overhead wise.
There was a problem hiding this comment.
We should clarify this in the next SIG, I think we've previously discussed it when we talked about how to handle 'optional'. I don't think that the 'must always be' wording in the documentation overrides the optional nature of proto3 fields as it makes no sense to add an element to a field that shouldn't be there.
In my view, a validator would first check if the field is present and then perform checks on that field. If there are no links in the message, then a message with a missing link_table should pass validation. If there are links in the message, then the link_table should be present and obey the documented semantics.
There was a problem hiding this comment.
In my view, a validator would first check if the field is present and then perform checks on that field. If there are no links in the message, then a message with a missing link_table should pass validation. If there are links in the message, then the link_table should be present and obey the documented semantics.
That makes things unnecessarily complex and error-prone. Insisting on having a default value without "thinking" thus makes sense to me.
optional nature of proto3 fields
The proto doesn't use the optional keyword any more. So an optional index boils down to have a zero index value and thus requires a default table entry. For example
// Reference to link in ProfilesDictionary.link_table. [optional]
// It can be unset / set to 0 if no link exists, as link_table[0] is always a 'null' default value.
int32 link_index = 5;
There was a problem hiding this comment.
We should clarify and not address this in this PR (which is about something else entirely).
There was a problem hiding this comment.
SGTM. Just suggested it as it would be a two-line change and the docs seemed to be clear.
mapping_table[0] should now be the only dummy mapping
Is there PR or fix for Devfiler on this? Seems Devfiler is now defunct unless I revert this change locally. |
|
4 participants
Summary
mapping_table[0] should now be the only dummy mapping. Previously, the agent generated a unique dummy mapping per frame.
Also see:
NOTE: Devfiler needs to be updated to account for dummy mappings.