Skip to content

EW-8447 Fix CPU profiling again#2571

Merged
harrishancock merged 5 commits intomainfrom
harris/EW-8447-fix-cpu-profiling-harder
Aug 21, 2024
Merged

EW-8447 Fix CPU profiling again#2571
harrishancock merged 5 commits intomainfrom
harris/EW-8447-fix-cpu-profiling-harder

Conversation

@harrishancock
Copy link
Copy Markdown
Collaborator

My initial attempt at a fix made a subtle behavior change which I did not consider fully: instead of a new incomingQueueNotifier XThreadNotifier being constructed for every inspector connection, my fix caused all inspector connections to re-use one long-lived XThreadNotifier. This subsequently ran afoul of the fact that XThreadNotifier::awaitNotification() is not cancel-safe: if it is cancelled while awaiting its stored promise, calling awaitNotification() a second time tries to await a moved-from promise.

This PR restores the previous behavior of creating a new incomingQueueNotifier XThreadNotifier for every inspector connection. However, instead of constructing it in the WebSocketIoHandler constructor as before, I moved the construction to the messageLoop() function implementation, which also spawns the dispatch loop. This narrows the scope of access to the notifier to only those functions which actually need it, keeps our usage of the dispatch kj::Executor localized in one place, and avoids synchronously blocking the inspector thread, as we would have had to do if we constructed it in the WebSocketIoHandler constructor.

The previous attempt at a test didn't fail because it didn't actually exercise the inspector protocol. If it had, it would have encounterd a WebSocket error and failed.

This commit removes the previous test, uncomments out the profiling test, factors it out into a separate function, then uses that function to implement two separate test cases, one to actually test profiling, and one to test repeated inspector connections.
My initial attempt at a fix made a subtle behavior change which I did not consider fully: instead of a new `incomingQueueNotifier` XThreadNotifier being constructed for every inspector connection, my fix caused all inspector connections to re-use one long-lived XThreadNotifier. This subsequently ran afoul of the fact that XThreadNotifier::awaitNotification() is not cancel-safe: if it is cancelled while awaiting its stored promise, calling awaitNotification() a second time tries to await a moved-from promise.

This commit restores the previous behavior of creating a new `incomingQueueNotifier` XThreadNotifier for every inspector connection. However, instead of constructing it in the WebSocketIoHandler constructor as before, I moved the construction to the `messageLoop()` function implementation, which also spawns the dispatch loop. This narrows the scope of access to the notifier to only those functions which actually need it, keeps our usage of the dispatch kj::Executor localized in one place, and avoids synchronously blocking the inspector thread, as we would have had to do if we constructed it in the WebSocketIoHandler constructor.

Fixes #2564.
This is no longer needed.
@harrishancock harrishancock requested review from a team as code owners August 21, 2024 13:28
@harrishancock harrishancock requested review from danlapid, garrettgu10, npaun and penalosa and removed request for garrettgu10 and npaun August 21, 2024 13:28
@harrishancock harrishancock force-pushed the harris/EW-8447-fix-cpu-profiling-harder branch from 6d4211d to a992145 Compare August 21, 2024 13:30
@harrishancock harrishancock merged commit 7e856d7 into main Aug 21, 2024
@harrishancock harrishancock deleted the harris/EW-8447-fix-cpu-profiling-harder branch August 21, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants