Replace std::unique_ptr with base::HeapArray in test_utils.cc [chromium/src : main]
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
1 comment
This is just a test so unlikely to impact our users 😊 but seems like a fine change in general.
Related details
- Mathijs Affourtit
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Primiano Tucci (Gerrit)
Primiano Tucci added 1 comment![]( "Open in Gerrit")
Frankly the benefit brought by this CL is IMHO nearly 0. Tge only thing this is adding is a CHECK around the assignment payload[kSize - 1], which was allocated 2 lines above. This is imho the sort extremely unpragmatic change which is 99% churn (let's not even mention that this is a test file).
More in general, I had a look at base::span, and I have to confess I'm worried about the liberal replacement of raw pointes into base-span all over the codebase. This make things look like an array, but introduces a CHECK on each [] access. If done liberally all over the places, this bloats binary size, will thrash icache and create extra load on the branch predictor (which are already pityful on arm's little cores).
In summary, no concerns for this CL (other than thinking this is pure churn), but overall worried if you are liberally replacing all raw arrays with base::span as that is going to be yet another papercut for chrome's performance.
Related details
- Mathijs Affourtit
- Primiano Tucci
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mathijs Affourtit (Gerrit)
Mathijs Affourtit added 1 comment![]( "Open in Gerrit")
Frankly the benefit brought by this CL is IMHO nearly 0. Tge only thing this is adding is a CHECK around the assignment payload[kSize - 1], which was allocated 2 lines above. This is imho the sort extremely unpragmatic change which is 99% churn (let's not even mention that this is a test file).
More in general, I had a look at base::span, and I have to confess I'm worried about the liberal replacement of raw pointes into base-span all over the codebase. This make things look like an array, but introduces a CHECK on each [] access. If done liberally all over the places, this bloats binary size, will thrash icache and create extra load on the branch predictor (which are already pityful on arm's little cores).
In summary, no concerns for this CL (other than thinking this is pure churn), but overall worried if you are liberally replacing all raw arrays with base::span as that is going to be yet another papercut for chrome's performance.
For reference the code health project doc is here:
https://docs.google.com/document/d/1YsPR8GoN8VTP1ABKCISaQkuBif1Cn80cTxTjsM8QT4s/edit?resourcekey=0-3qKCKgDwFwwjS7KI5XAcBQ&tab=t.0
@tse...@google.com would you be able to chime in here??
Related details
- Primiano Tucci
- Primiano Tucci
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Sepez (Gerrit)
Thomas Sepez added 3 comments![]( "Open in Gerrit")
memset(payload.data(), '.', kLargeMessageSize);this now becomes `std::ranges::fill(payload, ',');
payload[kLargeMessageSize - 1] = 0;this now becomes `payload.back() = 0;`
writer->NewTracePacket()->set_for_testing()->set_str(payload.data(),
kLargeMessageSize);This function should be converted to take span as an argument so as to just pass (payload) and have it implicitly convert to span.
Related details
- Mathijs Affourtit
- Primiano Tucci
- Primiano Tucci
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Sepez (Gerrit)
Thomas Sepez added 1 comment![]( "Open in Gerrit")
payload[kLargeMessageSize - 1] = 0;this now becomes `payload.back() = 0;`
actually `payload.span().back() = 0`.
Thomas Sepez (Gerrit)
Thomas Sepez added 1 comment![]( "Open in Gerrit")
writer->NewTracePacket()->set_for_testing()->set_str(payload.data(),
kLargeMessageSize);This function should be converted to take span as an argument so as to just pass (payload) and have it implicitly convert to span.
Ah, but it looks to be a generated protobuf method, but we can still clean up the call to be `set_str(payload.data(), payload.size())`
Mathijs Affourtit (Gerrit)
Mathijs Affourtit voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mathijs Affourtit
| Commit-Queue | +1 |
3 comments
this now becomes `std::ranges::fill(payload, ',');
Done
Thomas Sepezthis now becomes `payload.back() = 0;`
actually `payload.span().back() = 0`.
Done. Found that I needed to use .as_span(), rather than .span(). Let me know if that's not right.
writer->NewTracePacket()->set_for_testing()->set_str(payload.data(),
kLargeMessageSize);Thomas SepezThis function should be converted to take span as an argument so as to just pass (payload) and have it implicitly convert to span.
Ah, but it looks to be a generated protobuf method, but we can still clean up the call to be `set_str(payload.data(), payload.size())`
- Primiano Tucci
- Primiano Tucci
- Thomas Sepez
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mathijs Affourtit (Gerrit)
Mathijs Affourtit added 1 comment![]( "Open in Gerrit")
@prim...@chromium.org do you have any outstanding concerns on this particular change? If not, could you add your +1?
Related details
- Primiano Tucci
- Primiano Tucci
- Thomas Sepez
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Primiano Tucci (Gerrit)
Primiano Tucci voted and added 1 comment![]( "Open in Gerrit")
Votes added by Primiano Tucci
| Code-Review | +1 |
1 comment
No as I said I didn't have any conccerns with the CL itself since the beginning cause it's a test file. just the overall approach, but I said what I had to say on that document.
Related details
- Mathijs Affourtit
- Primiano Tucci
- Thomas Sepez
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mathijs Affourtit (Gerrit)
Mathijs Affourtit added 1 comment![]( "Open in Gerrit")
@prim...@chromium.org do you have any outstanding concerns on this particular change? If not, could you add your +1?
- Primiano Tucci
- Thomas Sepez
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mathijs Affourtit (Gerrit)
Mathijs Affourtit voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Replace std::unique_ptr with base::HeapArray in test_utils.cc
Code Health project. Using std::unique_ptr<T[]> does not automatically preserve the size of the allocation. This forces a need for ad-hoc bounds checks, which are easy to get wrong, leading to bugs which attackers use to compromise our users.
- M services/tracing/perfetto/test_utils.cc
Code-Review: +1 by Primiano Tucci, +1 by Stephen Nusko
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |