perf(uses-http2): check protocol first #2701
Conversation
| @@ -43,7 +43,7 @@ class UsesHTTP2Audit extends Audit { | |||
|
|
|||
| // Filter requests that are on the same host as the page and not over h2. | |||
| const resources = networkRecords.filter(record => { | |||
| const requestHost = new URL(record._url).host; | |||
| const requestHost = record._parsedURL && record._parsedURL.host; | |||
There was a problem hiding this comment.
netrequest.parsedURL uses a different implementation of url parsing. (Though I hope that wouldn't matter).
Alternatively we could add caching to our URL shim? That'd have the benefit of improving perf for all audits/gatherers.
Don't feel strongly, just raising the idea.
There was a problem hiding this comment.
this is the only place we were using new URL for network records over parsedURL, the only other place where we use new URL in a loop is for the anchor tags which I think is less likely to have hundreds/thousands of target=_blank elements, so maybe it's ok?
I'm not sure what the overlap of duplicate URLs would be for caching
There was a problem hiding this comment.
okay then.
add a comment to explain why we use URL.hostname but ParsedURL.host ? :)
|
@brendankenny The screenshot is the before, the after is |
|
what site are those numbers from? I can't get more than a few hundred URLs there so far :) |
|
@brendankenny just from cnn.com |
|
hmm, something weird is definitely happening in that case. For cnn I'm seeing 449 URL constructor calls in uses-http2, but 3940 constructor calls in the whole run, so in theory we're wasting 16 seconds just constructing URLs. But I'm also only seeing 50-250ms spent in that audit, so I'm not sure what's going on. Maybe the URL constructor's performance changed? I'm running node 8.1.4. |
|
@brendankenny not all constructor calls are created equal, there's nothing that wrong with a simple URL parsing but when we throw enormous data URLs in there it gets pretty slow pretty quick https://jsperf.com/new-url/1. The screenshot of 2s run was with an ad that had particularly many data URIs with node 8.1.2 vs <1 ms, is this an objection to the fix or just curiosity? |
ah, that wasn't clear before. In that case, another option would be to do an early return on |
|
I did not expect using parsedURL to be so controversial 😆 that's fine too, though it's still wasted parsing |
patrickhulce
changed the title
|
@brendankenny looks like you can take another look |
Invoking
new URLfor each network record was taking an insane amount of time