-
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(preload): use lantern to compute savings #5062
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
badbe11
core(preload): use lantern to compute savings
patrickhulce 51238e7
Merge branch 'master' into lantern_preload_opp
patrickhulce 1a03349
Merge branch 'master' into lantern_preload_opp
patrickhulce 08fa322
rebase and expand tests
patrickhulce 9d0f995
feedback
patrickhulce 0dff54d
add comment
patrickhulce 1b3c2a7
feedback
patrickhulce a2ef139
Merge branch 'master' into lantern_preload_opp
patrickhulce 2ec133c
weak smoke test
patrickhulce File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,10 @@ const TcpConnection = require('./tcp-connection'); | |
const DEFAULT_SERVER_RESPONSE_TIME = 30; | ||
const TLS_SCHEMES = ['https', 'wss']; | ||
|
||
// Each origin can have 6 simulatenous connections open | ||
// https://cs.chromium.org/chromium/src/net/socket/client_socket_pool_manager.cc?type=cs&q="int+g_max_sockets_per_group" | ||
const CONNECTIONS_PER_ORIGIN = 6; | ||
|
||
module.exports = class ConnectionPool { | ||
/** | ||
* @param {LH.WebInspector.NetworkRequest[]} records | ||
|
@@ -82,15 +86,26 @@ module.exports = class ConnectionPool { | |
throw new Error(`Could not find a connection for origin: ${origin}`); | ||
} | ||
|
||
// Make sure each origin has minimum number of connections available for max throughput | ||
while (connections.length < CONNECTIONS_PER_ORIGIN) connections.push(connections[0].clone()); | ||
|
||
this._connectionsByOrigin.set(origin, connections); | ||
} | ||
} | ||
|
||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a few lines to describe what's happening in here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
* This method finds an available connection to the origin specified by the network record or null | ||
* if no connection was available. If returned, connection will not be available for other network | ||
* records until release is called. | ||
* | ||
* If ignoreConnectionReused is true, acquire will consider all connections not in use as available. | ||
* Otherwise, only connections that have matching "warmth" are considered available. | ||
* | ||
* @param {LH.WebInspector.NetworkRequest} record | ||
* @param {{ignoreConnectionReused?: boolean}} options | ||
* @return {?TcpConnection} | ||
*/ | ||
acquire(record) { | ||
acquire(record, options = {}) { | ||
if (this._connectionsByRecord.has(record)) { | ||
// @ts-ignore | ||
return this._connectionsByRecord.get(record); | ||
|
@@ -99,16 +114,26 @@ module.exports = class ConnectionPool { | |
const origin = String(record.parsedURL.securityOrigin()); | ||
/** @type {TcpConnection[]} */ | ||
const connections = this._connectionsByOrigin.get(origin) || []; | ||
const wasConnectionWarm = !!this._connectionReusedByRequestId.get(record.requestId); | ||
const connection = connections.find(connection => { | ||
const meetsWarmRequirement = wasConnectionWarm === connection.isWarm(); | ||
return meetsWarmRequirement && !this._connectionsInUse.has(connection); | ||
}); | ||
// Sort connections by decreasing congestion window, i.e. warmest to coldest | ||
const availableConnections = connections | ||
.filter(connection => !this._connectionsInUse.has(connection)) | ||
.sort((a, b) => b.congestionWindow - a.congestionWindow); | ||
|
||
const observedConnectionWasReused = !!this._connectionReusedByRequestId.get(record.requestId); | ||
|
||
/** @type {TcpConnection|undefined} */ | ||
let connectionToUse = availableConnections[0]; | ||
if (!options.ignoreConnectionReused) { | ||
connectionToUse = availableConnections.find( | ||
connection => connection.isWarm() === observedConnectionWasReused | ||
); | ||
} | ||
|
||
if (!connectionToUse) return null; | ||
|
||
if (!connection) return null; | ||
this._connectionsInUse.add(connection); | ||
this._connectionsByRecord.set(record, connection); | ||
return connection; | ||
this._connectionsInUse.add(connectionToUse); | ||
this._connectionsByRecord.set(record, connectionToUse); | ||
return connectionToUse; | ||
} | ||
|
||
/** | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
seems like it gets most of the way to an opportunity but then stops here? Is measuring impact on TTI or whatever problematic?
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.
Yeah fair, I was mostly retaining parity with the old way, but we can do max(TTI, load) like the others 👍
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.
having had the hour to think about it, I've reconvinced myself that this is a bad idea and we should keep it to max(...wastedMs), our selection of preloaded items is fairly narrow and in a busy page it will rarely impact the longest chain
given our super ambiguous no-longer-just-on-a-particular-metric approach, it seems OK for now