core(lantern): use fixed times for data URLs #9932
Conversation
|
|
||
| beforeAll(async () => { | ||
| it('should compute a timeInMs', async () => { |
There was a problem hiding this comment.
cleaned this up because it made looking at just one test with .only impossible during development
There was a problem hiding this comment.
Funniest effect was a 15 second increase to the pessimistic TTFCPUI where the data URL time was just over the threshold before but now that we shortened it to a few ms that quiet window closed and now FCPUI comes after the second wave of activity instead 😆
That's amazing
| @@ -7,6 +7,7 @@ | |||
|
|
|||
| const BaseNode = require('./base-node.js'); | |||
| const NetworkRequest = require('../network-request.js'); | |||
| const URL = require('../url-shim.js'); | |||
There was a problem hiding this comment.
We've got to rename this thing at some point
| /** | ||
| * @return {boolean} | ||
| */ | ||
| get isNonNetworkProtocol() { |
There was a problem hiding this comment.
What's up with all getters in this file? Kind of weird :)
| // 2ms per request + 10ms/MB | ||
| // @see traces on https://dopiaza.org/tools/datauri/examples/index.php | ||
| const sizeInMb = (record.resourceSize || 0) / 1024 / 1024; | ||
| timeElapsed = 2 + 10 * sizeInMb - timingData.timeElapsed; |
There was a problem hiding this comment.
Is this low compared to @paulirish's 10-15ms suggestion?
There was a problem hiding this comment.
yeah but I couldn't find evidence to support that much overhead on some test pages and 10-15 seemed too high compared to our read from disk estimates, @paulirish do you have any docs we could link to to support 10-15ms?
happy to use whatever we have clear evidence of
There was a problem hiding this comment.
happy to use whatever we have clear evidence of
I'd guess that @paulirish was ballparking, too, so let's call this good and we can revisit later if somehow these end up giving us bizarre lantern graphs or something (seems very unlikely :)
There was a problem hiding this comment.
yah was just ballparking. I went looking in UMA yesterday but couldnt spot a relevant histogram.
Seems fine for now. 👍
| * @return {boolean} | ||
| */ | ||
| get isNonNetworkProtocol() { | ||
| return URL.NON_NETWORK_PROTOCOLS.includes(this._record.protocol); |
There was a problem hiding this comment.
Is it useful to reuse this here vs making a new list here? There are some non network protocols we wouldn't include here (presumably eg intent wouldn't end up as a node, but this list might expand in the future)
There was a problem hiding this comment.
I'm not sure when this would actually need to diverge from our canonical set, so I'd prefer to reuse. It's true it will include things we don't actually need to test but I doubt the reverse will ever be true
| const simulator = new Simulator({serverResponseTimeByOrigin}); | ||
| const result = simulator.simulate(nodeA); | ||
|
|
||
| // should be ~2ms for A, ~12ms for B |
There was a problem hiding this comment.
for clarity for future tinkerers, maybe
| // should be ~2ms for A, ~12ms for B | |
| // should be ~2ms for A (resourceSize 0), ~12ms for B (resourceSize 1MB) |
...or else a comment up on the node creation lines calling out the resourceSize?
Summary
Special cases non-network protocols in simulator similar to how we special case cached responses. Net effect on our checked-in data overall was minimal (moving just 1 URL from OK to Good).
Funniest effect was a 15 second increase to the pessimistic TTFCPUI where the data URL time was just over the threshold before but now that we shortened it to a few ms that quiet window closed and now FCPUI comes after the second wave of activity instead 😆 everything else was very small movement.
Related Issues/PRs
closes #9833