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
misc: simplifications in simulator/connection-pool #7894
Conversation
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.
LGTM other than the queuedTime :)
const observedConnectionWasReused = !!this._connectionReusedByRequestId.get(record.requestId); | ||
/** @type {TcpConnection[]} */ |
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 isn't necessary anymore?
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 isn't necessary anymore?
no, its inferred from the first part of the init and the _findAvailableConnectionWithLargestCongestionWindow
callsite
*/ | ||
_markNodeAsReadyToStart(node, queuedTime) { |
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.
I'd prefer to keep this, it's pretty helpful when stepping through the debugger
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.
I'd prefer to keep this, it's pretty helpful when stepping through the debugger
gotcha :)
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.
LGTM!
@@ -503,7 +501,7 @@ module.exports = Simulator; | |||
* @typedef NodeTimingIntermediate | |||
* @property {number} [startTime] | |||
* @property {number} [endTime] | |||
* @property {number} [queuedTime] | |||
* @property {number} [queuedTime] Helpful for debugging. |
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.
👍
another one I accumulated while stepping through the simulator. Feel free to reject anything @patrickhulce :)
remove
records
fromLH.Artifacts.NetworkAnalysis
. The caller of the computed artifact has to already have them to get theNetworkAnalysis
, and we never use them on any instance of the type anyways.early return in
PageDependencyGraph.linkNetworkNodes()
for the common case where there were no redirects of a particular recordsplit
ConnectionPool.acquire()
into the case where a connection is needed to fetch a record and the case where a connection is already fetching a record.This seems ok as a place where the generality hasn't proven necessary...we only call
acquire()
in three places, and the two done that return assigned connections already explicitly call out with a comment and a type cast that it knows a connection already exists for this record.removed
NodeTimingIntermediate.queuedTime
as it isn't used for anything. I can add back if this was useful for debugging or something.