-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core(network-request): simplify recomputeTimesWithResourceTiming #15107
Conversation
this.responseHeadersEndTime = Math.max(this.responseHeadersEndTime, this.networkRequestTime); | ||
|
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.
when comparing headersReceivedTime
with the value of this.responseHeadersEndTime
after, for every run I tried (including lantern database) there is no difference. This code originally came from CDT (no explanation there) - best I can reason is that it was to be extra careful that ordering invariants are maintained. I don't think it's ever actually needed, though. Adding a comment here for better readability.
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.
love it. this readability refactor makes me happy.
also... the DT logic here is still the same today fwiw. https://github.com/ChromeDevTools/devtools-frontend/blob/23cde35228250b831d70b63ab76d05ab4d193129/front_end/core/sdk/NetworkRequest.ts#L737-L754
// Take networkRequestTime and responseHeadersEndTime from timing data for better accuracy. | ||
// Timing's requestTime is a baseline in seconds, rest of the numbers there are ticks in millis. | ||
this.networkRequestTime = timing.requestTime * 1000; | ||
const headersReceivedTime = this.networkRequestTime + timing.receiveHeadersEnd; | ||
if (!this.responseHeadersEndTime || this.responseHeadersEndTime < 0) { | ||
this.responseHeadersEndTime = headersReceivedTime; | ||
} |
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.
this literally cannot be true, the one place this is called sets responseHeadersEndTime
to a non-zero value first.
@@ -372,16 +373,22 @@ class NetworkRequest { | |||
// Don't recompute times if the data is invalid. RequestTime should always be a thread timestamp. | |||
// If we don't have receiveHeadersEnd, we really don't have more accurate data. | |||
if (timing.requestTime === 0 || timing.receiveHeadersEnd === -1) return; | |||
|
|||
// Take networkRequestTime and responseHeadersEndTime from timing data for better accuracy. |
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.
// Take networkRequestTime and responseHeadersEndTime from timing data for better accuracy. | |
// networkRequestTime and responseHeadersEndTime are canonically defined in resource timing. Up till now, responseHeadersEndTime has a bogus fallback that was set in _onResponse(). |
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.
lmk if the new comment is still lacking for u
this.responseHeadersEndTime = Math.max(this.responseHeadersEndTime, this.networkRequestTime); | ||
|
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.
love it. this readability refactor makes me happy.
also... the DT logic here is still the same today fwiw. https://github.com/ChromeDevTools/devtools-frontend/blob/23cde35228250b831d70b63ab76d05ab4d193129/front_end/core/sdk/NetworkRequest.ts#L737-L754
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.
Looks good, just one quick tangent...
@@ -17,7 +17,7 @@ fi | |||
|
|||
printf "Determined the following files have been touched:\n\n$CHANGED_FILES\n\n" | |||
|
|||
if ! echo $CHANGED_FILES | grep -E 'dependency-graph|metrics|lantern|predictive-perf|network-recorder' > /dev/null; then | |||
if ! echo $CHANGED_FILES | grep -E 'dependency-graph|metrics|lantern|predictive-perf|network-recorder|network-request' > /dev/null; then |
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.
How valuable is this check? Are we worried about the extra CI compute time it would take to run this on every PR?
I'm worried that this list is incomplete, or that we may forget to update this list when adding new lantern relevant files to the repo.
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.
smokes def. take most of the time of a run, and this is only ~2m. mostly worried about the constant downloading of this thing and how that could upset cloud storage?
Adds a comment and removes a conditional that is impossible to be true.