Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23308009:
[DevTools] Save as HAR: classify time before network activity as "blocked". (Closed)
|
Created:
7 years, 3 months ago by eustas Modified:
7 years, 3 months ago Reviewers:
caseq CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, eae+blinkwatch, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
Description[DevTools] Save as HAR: classify time before network activity as "blocked".
EOM
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157971
Patch Set 1 #Patch Set 2 : Add custom field for "other" time #Patch Set 3 : Fix rounding, remove proxy from network activities list. #
Total comments: 10
Patch Set 4 : #
Total comments: 10
Patch Set 5 : Addressed comments #
Total comments: 8
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Messages
Total messages: 13 (0 generated)
not lgtm Why are we doing this? Per HAR 1.2 spec, blocked is "Time spent in a queue waiting for a network connection."
Please, take another look.
https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... File Source/devtools/front_end/HAREntry.js (right): https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/HAREntry.js:53: var time = Math.max(timings.blocked, 0) + Math.max(timings.dns, 0) + Math.max(timings.connect, 0) + Math.max(timings.send, 0) + Math.max(timings.wait, 0) + Math.max(timings.receive, 0); So if we lost some time due to imperfect instrumentation, we will report something different from whatever is show in the Network panel and different from the wall time. I don't think that's right. https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/HAREntry.js:149: if (typeof time === "number" && time !== -1) Note that if (time >= 0) times.push(time) would work as well. https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/HAREntry.js:153: blocked = Math.min.apply(null, times); So what happened to the old logic of "if connection is reused, blocked time is the time we waited for connection"? Also, this loop is somewhat difficult to read. https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/HAREntry.js:240: return typeof startTime !== "number" || startTime === -1 ? -1 : (timing[end] - startTime); Please remove redundant parenthesis. https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/HAREntry.js:278: return time === -1 ? -1 : (time * 1000); Ditto.
PTAL https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... File Source/devtools/front_end/HAREntry.js (right): https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/HAREntry.js:53: var time = Math.max(timings.blocked, 0) + Math.max(timings.dns, 0) + Math.max(timings.connect, 0) + Math.max(timings.send, 0) + Math.max(timings.wait, 0) + Math.max(timings.receive, 0); On 2013/08/30 15:12:53, caseq wrote: > So if we lost some time due to imperfect instrumentation, we will report > something different from whatever is show in the Network panel and different > from the wall time. I don't think that's right. Done. https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/HAREntry.js:149: if (typeof time === "number" && time !== -1) On 2013/08/30 15:12:53, caseq wrote: > Note that if (time >= 0) times.push(time) would work as well. Done. https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/HAREntry.js:153: blocked = Math.min.apply(null, times); On 2013/08/30 15:12:53, caseq wrote: > So what happened to the old logic of "if connection is reused, blocked time is > the time we waited for connection"? > Also, this loop is somewhat difficult to read. It is baked into glue. https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/HAREntry.js:240: return typeof startTime !== "number" || startTime === -1 ? -1 : (timing[end] - startTime); On 2013/08/30 15:12:53, caseq wrote: > Please remove redundant parenthesis. Done. https://codereview.chromium.org/23308009/diff/7001/Source/devtools/front_end/... Source/devtools/front_end/HAREntry.js:278: return time === -1 ? -1 : (time * 1000); On 2013/08/30 15:12:53, caseq wrote: > Ditto. Done.
https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... File Source/devtools/front_end/HAREntry.js (right): https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:136: function nextEventTime(events) { style: { => next line https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:138: if (timing[events[i]] > -1) nit: -1 is a "null" value, so I think it would be better to either compare using "!== -1" or ">= 0", not "> -1". https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:147: if (timing["dnsStart"] > -1) Here ad below, why not timing.dnsStart? We should take advantage of js compiler's type checking when we can. https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:152: connect = Math.round(timing["sendStart"]) - Math.round(timing["connectStart"]); On a closer look to the spec, do we really have to round times? They're specified as "number". https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:154: var send = Math.round(timing["sendEnd"]) - Math.round(timing["sendStart"]); Are we intentionally counting send time from sendStart, not from previous event?
PTAL https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... File Source/devtools/front_end/HAREntry.js (right): https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:136: function nextEventTime(events) { On 2013/09/13 15:24:07, caseq wrote: > style: { => next line Done. https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:138: if (timing[events[i]] > -1) On 2013/09/13 15:24:07, caseq wrote: > nit: -1 is a "null" value, so I think it would be better to either compare using > "!== -1" or ">= 0", not "> -1". Done. https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:147: if (timing["dnsStart"] > -1) On 2013/09/13 15:24:07, caseq wrote: > Here ad below, why not timing.dnsStart? We should take advantage of js > compiler's type checking when we can. Done. https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:152: connect = Math.round(timing["sendStart"]) - Math.round(timing["connectStart"]); On 2013/09/13 15:24:07, caseq wrote: > On a closer look to the spec, do we really have to round times? They're > specified as "number". Done. https://codereview.chromium.org/23308009/diff/11001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:154: var send = Math.round(timing["sendEnd"]) - Math.round(timing["sendStart"]); On 2013/09/13 15:24:07, caseq wrote: > Are we intentionally counting send time from sendStart, not from previous event? Yes. Last event (block, dns or connect) lasts till sendStart.
https://codereview.chromium.org/23308009/diff/17001/Source/devtools/front_end... File Source/devtools/front_end/HAREntry.js (right): https://codereview.chromium.org/23308009/diff/17001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:148: if (timing.dnsStart > -1) nit: >= 0 https://codereview.chromium.org/23308009/diff/17001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:149: dns = nextEventTime(["connectStart", "sendStart"]) - timing.dnsStart; So, is it possible for us to have dns time while not having connect time? https://codereview.chromium.org/23308009/diff/17001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:152: if (timing.connectStart > -1) as discussed, >= 0 https://codereview.chromium.org/23308009/diff/17001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:160: if (timing.sslStart > -1 && timing.sslEnd > -1) as discussed, >= 0
PTAL https://codereview.chromium.org/23308009/diff/17001/Source/devtools/front_end... File Source/devtools/front_end/HAREntry.js (right): https://codereview.chromium.org/23308009/diff/17001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:148: if (timing.dnsStart > -1) On 2013/09/17 16:56:22, caseq wrote: > nit: >= 0 Done. https://codereview.chromium.org/23308009/diff/17001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:149: dns = nextEventTime(["connectStart", "sendStart"]) - timing.dnsStart; On 2013/09/17 16:56:22, caseq wrote: > So, is it possible for us to have dns time while not having connect time? Not sure. I've tried to avoid any assumptions here. https://codereview.chromium.org/23308009/diff/17001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:152: if (timing.connectStart > -1) On 2013/09/17 16:56:22, caseq wrote: > as discussed, >= 0 Done. https://codereview.chromium.org/23308009/diff/17001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:160: if (timing.sslStart > -1 && timing.sslEnd > -1) On 2013/09/17 16:56:22, caseq wrote: > as discussed, >= 0 Done.
lgtm https://codereview.chromium.org/23308009/diff/22001/Source/devtools/front_end... File Source/devtools/front_end/HAREntry.js (right): https://codereview.chromium.org/23308009/diff/22001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:142: return -1; nit: console.assert(false, ...) here, as the call-sites seem to imply this never happens.
https://codereview.chromium.org/23308009/diff/22001/Source/devtools/front_end... File Source/devtools/front_end/HAREntry.js (right): https://codereview.chromium.org/23308009/diff/22001/Source/devtools/front_end... Source/devtools/front_end/HAREntry.js:142: return -1; On 2013/09/18 13:49:40, caseq wrote: > nit: console.assert(false, ...) here, as the call-sites seem to imply this never > happens. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eustas@chromium.org/23308009/27001
Message was sent while issue was closed.
Change committed as 157971 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
