spanification: automatically spanify .../tracing/perfetto/privacy_filtering_check.cc etc. [chromium/src : main]
Daniel Angulo (Gerrit)
Daniel Angulo voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 10 comments![]( "Open in Gerrit")
size_t acceptedFieldIdSize(const int* accepted_field_ids) {
size_t size = 0;
while (UNSAFE_TODO(accepted_field_ids[size]) != -1) {
size++;
}
return size;
}This is highly performance sensitive code, I think we shouldn't be computing the size. As I said I think we should keep this as unsafe until we update the generator to use bounds checked objects.
// SAFETY: proto is a pointer to the serialized trace proto which is a
// kProtoZeroMessageSet, which is a repeated field of
// TracePacket. TracePacket has a maximum size of 256 MiB - far below the
// threshold where we would need to use unsafe buffers.What does this mean about "far below the threshold"? This comment doesn't explain why this is safe? I think the fact is ProtoDecoder is a span like type, and it doesn't do bounds checking, at the moment it has 1 unsafe constructor and 2 safe-ish constructors, we can't claim UNSAFE_BUFFERS here and wash our hands of it. Lets leave it as UNSAFE_TODO.
auto proto_span = UNSAFE_BUFFERS(base::span(Also this type is non-obvious type so auto hides the information.
```
base::span<uint8_t> proto_span = UNSAFE_TODO(base:span(proto->begin(), proto->bytes_left()));
```
proto->begin(), static_cast<size_t>(proto->end() - proto->begin())));use bytes_left()
// SAFETY: MessageInfo states that the accepted_field_id is terminated byCan you link to this?
root->accepted_field_ids,It feels like we should spanify this, but this is coming from generated code see (go/how-do-i-chrometto), so for now this needs to remain UNSAFE_TODO.
output.append(proto_span.subspan(current_field_start).data(),
proto_span.subspan(next_field_start).data());This doesn't look great (also two subspans being created)
```
const std::string_view msg_data =
base::as_string_view(proto_span.subspan(current_field_start, next_field_start - current_field_start));
output.append(msg_data.begin(), msg_data.end());
```
Perhaps?
Actually thinking about this a bit more we are just looping over `proto` and reading one by one, we can instead of creating a bunch of subspans just use SpanReader?
I.E.
```
base::SpanReader reader (proto_span);
size_t current_field_start = 0;
size_t next_field_start = 0;
for (auto f = proto->ReadField(); f.valid();
f = proto->ReadField(), current_field_start = next_field_start) {
next_field_start = proto->read_offset();
CHECK_EQ(reader.num_read(), current_field_start);
...
maybe_data = reader.Read(next_field_start - current_field_start);
output.append(maybe_data->begin(), maybe_data->end());
```
.copy_from(base::as_byte_span(output).subspan(out_msg_start_offset,note: this has to remain copy_from (since it is potentially overlapping).
.copy_from(base::span(field_id_buf).first(field_id_length));copy_from_nonoverlapping
.copy_from(base::span(payload_size_buf).first(payload_size_length));copy_from_nonoverlapping
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
.copy_from(base::span(field_id_buf).first(field_id_length));copy_from_nonoverlapping
Hi @nus...@chromium.org, I have a different opinion about `copy_from`/`copy_from_nonoverlapping`. I don't really believe there are meaningful performance difference and we should use the short/safe version by default.
I added some arguments below:
https://chromium-review.googlesource.com/c/chromium/src/+/7541348/comment/7892b7db_f5eb5c23/
WDYT?
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 8 comments![]( "Open in Gerrit")
size_t acceptedFieldIdSize(const int* accepted_field_ids) {
size_t size = 0;
while (UNSAFE_TODO(accepted_field_ids[size]) != -1) {
size++;
}
return size;
}This is highly performance sensitive code, I think we shouldn't be computing the size. As I said I think we should keep this as unsafe until we update the generator to use bounds checked objects.
Done
// SAFETY: proto is a pointer to the serialized trace proto which is a
// kProtoZeroMessageSet, which is a repeated field of
// TracePacket. TracePacket has a maximum size of 256 MiB - far below the
// threshold where we would need to use unsafe buffers.What does this mean about "far below the threshold"? This comment doesn't explain why this is safe? I think the fact is ProtoDecoder is a span like type, and it doesn't do bounds checking, at the moment it has 1 unsafe constructor and 2 safe-ish constructors, we can't claim UNSAFE_BUFFERS here and wash our hands of it. Lets leave it as UNSAFE_TODO.
I leave it as UNSAFE_TODO for now.
Also this type is non-obvious type so auto hides the information.
```
base::span<uint8_t> proto_span = UNSAFE_TODO(base:span(proto->begin(), proto->bytes_left()));
```
Done
proto->begin(), static_cast<size_t>(proto->end() - proto->begin())));Daniel Angulouse bytes_left()
Done
// SAFETY: MessageInfo states that the accepted_field_id is terminated byCan you link to this?
Done
It feels like we should spanify this, but this is coming from generated code see (go/how-do-i-chrometto), so for now this needs to remain UNSAFE_TODO.
Done
output.append(proto_span.subspan(current_field_start).data(),
proto_span.subspan(next_field_start).data());This doesn't look great (also two subspans being created)
```
const std::string_view msg_data =
base::as_string_view(proto_span.subspan(current_field_start, next_field_start - current_field_start));
output.append(msg_data.begin(), msg_data.end());
```Perhaps?
Actually thinking about this a bit more we are just looping over `proto` and reading one by one, we can instead of creating a bunch of subspans just use SpanReader?
I.E.
```
base::SpanReader reader (proto_span);
size_t current_field_start = 0;
size_t next_field_start = 0;
for (auto f = proto->ReadField(); f.valid();
f = proto->ReadField(), current_field_start = next_field_start) {
next_field_start = proto->read_offset();
CHECK_EQ(reader.num_read(), current_field_start);
...
maybe_data = reader.Read(next_field_start - current_field_start);
output.append(maybe_data->begin(), maybe_data->end());
```
Done
.copy_from(base::span(payload_size_buf).first(payload_size_length));Daniel Angulocopy_from_nonoverlapping
Done
Related details
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 2 comments![]( "Open in Gerrit")
Let me know when the tests are passing.
.copy_from(base::span(field_id_buf).first(field_id_length));copy_from_nonoverlapping
Hi @nus...@chromium.org, I have a different opinion about `copy_from`/`copy_from_nonoverlapping`. I don't really believe there are meaningful performance difference and we should use the short/safe version by default.
I added some arguments below:
https://chromium-review.googlesource.com/c/chromium/src/+/7541348/comment/7892b7db_f5eb5c23/WDYT?
Interesting let me reply there, I'm happy to go with copy_from as a default, I had been operating as a clearly different buffers should be done with non_overlapping to ensure maximum performance to avoid impact due to our rewrites.
In this code we're finishing a trace which isn't super performance critical happy to go with the safe versions here.
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |