[Inspector] Socket backend: don't initialize the socket library when the RemoteInspector singleton is created - #470
dylan-conway wants to merge 2 commits into
Conversation
…the RemoteInspector singleton is created RemoteInspector::singleton() is constructed the first time any JSGlobalObject is created (JSGlobalObjectDebuggable -> RemoteControllableTarget::init), and its constructor called Socket::init(). On Windows that is WSAStartup(), which loads ws2_32/mswsock and costs the better part of a millisecond in every process, whether or not an inspector server or client ever exists. RemoteInspectorSocketEndpoint already calls Socket::init() when it is created, and every socket operation goes through the endpoint, so the call in the constructor is redundant.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 minutes Limit details: You’ve used the included review currently available. Your 61 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — removing the redundant Socket::init() from the RemoteInspector constructor is safe and saves the WSAStartup cost on every process start.
What was reviewed:
- Confirmed
RemoteInspectorSocketEndpoint::RemoteInspectorSocketEndpoint()still callsSocket::init()before any socket use. - Verified all
RemoteInspectorConnectionClientsocket paths (connectInet,createClient,send, dtor) go through the endpoint singleton, so init is guaranteed before first socket op. RemoteInspectorServerhas its ownSocket::init(), so the server path is also covered independently.
Extended reasoning...
Overview
Single-line functional change in Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp: the Socket::init() call is removed from RemoteInspector::RemoteInspector() and replaced with an explanatory comment. The constructor now only calls start(), which just sets m_enabled = true under a lock.
Security risks
None. This is initialization ordering for the platform socket library (WSAStartup on Windows, no-op on POSIX). No auth, crypto, permissions, or user-controlled data is involved. The only theoretical risk would be a socket call before WSAStartup, which is ruled out below.
Level of scrutiny
Low. The change is a one-line deletion in the remote-inspector socket backend, guarded by ENABLE(REMOTE_INSPECTOR). The correctness argument is easy to verify statically:
RemoteInspectorinherits fromRemoteInspectorConnectionClient, and every socket-touching method on that base (connectInet,createClient,send, and the destructor'sinvalidateClient) first obtainsRemoteInspectorSocketEndpoint::singleton().RemoteInspectorSocketEndpoint's constructor callsSocket::init()as its first statement (RemoteInspectorSocketEndpoint.cpp:55), before creating the wakeup socket pair and worker thread.RemoteInspectorServerseparately callsSocket::init()in its own constructor, so the listen path is covered too.WSAStartupis reference-counted, so dropping one redundant caller is harmless even when the inspector is used.
Therefore no socket API can be reached without Socket::init() having run, and the removed call was strictly redundant.
Other factors
The PR description gives a clear, measured motivation (~0.75 ms of Bun startup on Windows via JSGlobalObject::init → RemoteControllableTarget::init → registerTarget). The remaining start() call is unchanged and does no I/O. No prior human review comments or outstanding requests on the PR. The bug-hunting system found no issues.
Preview Builds
|
There was a problem hiding this comment.
LGTM — removing the redundant Socket::init() call is safe since RemoteInspectorSocketEndpoint already handles it.
What was reviewed:
- Verified
RemoteInspectorSocketEndpoint::RemoteInspectorSocketEndpoint()callsSocket::init()and all inspector socket ops (connectInet,createClient,send) route through the endpoint singleton. - Confirmed
Socket::init()on Windows isstd::once_flag-guarded, so the prior double-init was harmless but wasteful;start()does no socket work. - Checked that
RemoteInspector::connect(ConnectionID)receives an ID minted by the endpoint, so the endpoint is always constructed first.
Extended reasoning...
Overview
This PR removes a single Socket::init() call from the RemoteInspector constructor in the socket-backend implementation (RemoteInspectorSocket.cpp) and replaces it with an explanatory comment. The motivation is startup performance: the RemoteInspector singleton is constructed on the first JSGlobalObject creation regardless of whether remote inspection is ever used, and on Windows Socket::init() calls WSAStartup(), costing ~0.75 ms per process start.
Security risks
None. This defers Winsock initialization; it does not change what is initialized or expose any new surface. The inspector socket path is unchanged in behavior once actually used.
Level of scrutiny
Low. This is a one-line deletion in debug/inspector infrastructure. The key correctness question is whether any socket operation can be reached without first constructing RemoteInspectorSocketEndpoint. I verified:
RemoteInspectorSocketEndpoint::RemoteInspectorSocketEndpoint()callsSocket::init()(line 55).- Every socket-touching path in
RemoteInspectorConnectionClient(connectInet,createClient,send, destructor) andRemoteInspectorServerfirst callsRemoteInspectorSocketEndpoint::singleton(). RemoteInspector::start()only flipsm_enabledunder a lock — no socket work.Socket::init()on Windows is guarded bystd::once_flag, so the removed call was purely redundant, not load-bearing for ordering.
Other factors
The change is self-contained, the added comment accurately documents why the call was removed, and preview builds succeeded. No outstanding reviewer comments. The bug hunting system found no issues.
|
I think we can just compile without this. |
2 participants
With
ENABLE(REMOTE_INSPECTOR)and the socket backend,RemoteInspector::singleton()is constructed the first time anyJSGlobalObjectis created (JSGlobalObjectDebuggable::init→RemoteControllableTarget::init→registerTarget), and its constructor calledSocket::init(). On Windows that isWSAStartup(), which loads ws2_32 and friends — about 0.75 ms of every Bun process start (6.3 samples at 8 kHz underJSGlobalObject::init), whether or not an inspector server or client ever exists.RemoteInspectorSocketEndpointalready callsSocket::init()in its constructor, and every socket operation the inspector performs goes through the endpoint singleton, so the call in theRemoteInspectorconstructor is redundant. This removes it;start()(which only flipsm_enabled) stays.