NQE: make NQE::NotifyHeadersReceived async [chromium/src : main]
Martin Pan-Verde (Gerrit)
Martin Pan-Verde voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keita Suzuki
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keita Suzuki (Gerrit)
Keita Suzuki added 6 comments![]( "Open in Gerrit")
overall looks good logic wise. I would defer the review to the //net owners for other parts.
CC @yoi...@chromium.org as the implementer of the NotifyStartTransaction async. This experiment sort of works together especially the defer all steps option. Could you also take a look and see if this makes sense?
std::unordered_map<raw_ptr<const URLRequest>, base::TimeTicks>I think it would be fine to keep it as is, but we probably would want to combine the logic with `waiting_async_notify_start_transactions_` in the near future.
It might be good to leave a comment.
// Notifies NetworkQualityEstimator that the response header of |request| hasnit: I think we use ` instead of | now.
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyStartTransaction)) {
WaitNotifyStartTransactionDone(request);
}
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyHeadersReceived)) {
WaitNotifyHeadersReceivedDone(request);
}
Could we split this into a different method / function so that we do not have to copy for each subsequent method?
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyStartTransaction)) {
WaitNotifyStartTransactionDone(request);
}
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyHeadersReceived)) {
WaitNotifyHeadersReceivedDone(request);
}ditto
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyStartTransaction)) {
WaitNotifyStartTransactionDone(request);
}
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyHeadersReceived)) {
WaitNotifyHeadersReceivedDone(request);
}ditto
Related details
- Martin Pan-Verde
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. |
Martin Pan-Verde (Gerrit)
Martin Pan-Verde added 5 comments![]( "Open in Gerrit")
std::unordered_map<raw_ptr<const URLRequest>, base::TimeTicks>I think it would be fine to keep it as is, but we probably would want to combine the logic with `waiting_async_notify_start_transactions_` in the near future.
It might be good to leave a comment.
Done
// Notifies NetworkQualityEstimator that the response header of |request| hasnit: I think we use ` instead of | now.
Done
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyStartTransaction)) {
WaitNotifyStartTransactionDone(request);
}
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyHeadersReceived)) {
WaitNotifyHeadersReceivedDone(request);
}
Could we split this into a different method / function so that we do not have to copy for each subsequent method?
Done
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyStartTransaction)) {
WaitNotifyStartTransactionDone(request);
}
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyHeadersReceived)) {
WaitNotifyHeadersReceivedDone(request);
}Martin Pan-Verdeditto
Done
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyStartTransaction)) {
WaitNotifyStartTransactionDone(request);
}
if (base::FeatureList::IsEnabled(
kNetworkQualityEstimatorAsyncNotifyHeadersReceived)) {
WaitNotifyHeadersReceivedDone(request);
}- Keita Suzuki
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Pan-Verde (Gerrit)
Martin Pan-Verde voted Commit-Queue+1![]( "Open in Gerrit")
- Keita Suzuki
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoichi Osato (Gerrit)
Yoichi Osato added 2 comments![]( "Open in Gerrit")
Since this is complex, it is good to add testing/variations/fieldtrial_testing_config.json testing here.
kDeferCurrentStep,I'm not sure we need this kDeferCurrentStep flag, this makes code/experiment complex.
I think we might just consider running current NotifyHeadersReceived() body including kNetworkQualityEstimatorAsyncNotifyStartTransaction guard as-is, async, or defer independently to NetworkQualityEstimatorAsyncNotifyStartTransaction feature.
Then you don't need to care about the flag (and you can reuse my code?)
Related details
- Keita Suzuki
- Martin Pan-Verde
- Nidhi Jaju
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. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 4 comments![]( "Open in Gerrit")
Since this is complex, it is good to add testing/variations/fieldtrial_testing_config.json testing here.
+1, or if it's possible to add tests for this new behavior that would be great too!
// NotifyStartTransaction, then NotifyHeadersReceived.We should update this comment to say something similar to the above function comments, because we're not actually waiting for NotifyStartTransaction, then NotifyHeadersReceived. What the function actually does is immediately call NotifyStartTransactionInternal and NotifyHeadersReceivedInternal, if they haven't been called already.
// `params` contains theI love the cleanup, but would you mind separating this out into a separate CL as it makes it easier to review the changes specific to this feature?
Similar to https://chromium.googlesource.com/chromium/src/+/HEAD/docs/cl_tips.md#separate-behavior-changes-from-refactoring.
kDeferCurrentStep,I'm not sure we need this kDeferCurrentStep flag, this makes code/experiment complex.
I think we might just consider running current NotifyHeadersReceived() body including kNetworkQualityEstimatorAsyncNotifyStartTransaction guard as-is, async, or defer independently to NetworkQualityEstimatorAsyncNotifyStartTransaction feature.
Then you don't need to care about the flag (and you can reuse my code?)
+1, can you explain the value of experimenting with just deferring the previous step and running it offset by 1 event?
Related details
- Keita Suzuki
- Martin Pan-Verde
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. |
Martin Pan-Verde (Gerrit)
Martin Pan-Verde added 5 comments![]( "Open in Gerrit")
Nidhi JajuSince this is complex, it is good to add testing/variations/fieldtrial_testing_config.json testing here.
+1, or if it's possible to add tests for this new behavior that would be great too!
Removed some complexity and added the fieldtrial_testing_config.json and it uncovered test failures; rerunning CQ with the fix now...
We should update this comment to say something similar to the above function comments, because we're not actually waiting for NotifyStartTransaction, then NotifyHeadersReceived. What the function actually does is immediately call NotifyStartTransactionInternal and NotifyHeadersReceivedInternal, if they haven't been called already.
Done
I love the cleanup, but would you mind separating this out into a separate CL as it makes it easier to review the changes specific to this feature?
Similar to https://chromium.googlesource.com/chromium/src/+/HEAD/docs/cl_tips.md#separate-behavior-changes-from-refactoring.
Sounds good, created https://chromium-review.googlesource.com/c/chromium/src/+/7590613.
kDeferCurrentStep,Nidhi JajuI'm not sure we need this kDeferCurrentStep flag, this makes code/experiment complex.
I think we might just consider running current NotifyHeadersReceived() body including kNetworkQualityEstimatorAsyncNotifyStartTransaction guard as-is, async, or defer independently to NetworkQualityEstimatorAsyncNotifyStartTransaction feature.
Then you don't need to care about the flag (and you can reuse my code?)
+1, can you explain the value of experimenting with just deferring the previous step and running it offset by 1 event?
I added it mainly out of curiosity, but you're right that it made the code more complex and likely won't make that much of a difference. Changed the feature to be similar to kNetworkQualityEstimatorAsyncNotifyStartTransaction.
base::UmaHistogramLongTimes100(
base::StrCat({"NQE.RTT.ObservationBufferLifeTime2.",
CategoryToString(observation_category)}),
delta);
base::UmaHistogramLongTimes100("NQE.RTT.ObservationBufferLifeTime2.All",
delta);Adding the ability to add observations out of order could potentially change the deltas we see for these metrics - is that concerning? Or do you think it's fine as long as we pay attention to them during the experiment and fix if needed?
Related details
- Keita Suzuki
- Nidhi Jaju
- Yoichi Osato
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. |