Refactor CDP #422
Open
Refactor CDP #422
Conversation
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
Adding HTTP & websocket awareness to the TCP server. HTTP server handles `GET /json/version` and websocket upgrade requests. Conceptually, websocket handling is the same code as before, but receiving data will parse the websocket frames and writing data will wrap it in a websocket frame. The previous `Ctx` was split into a `Server` and a `Client`. This was largely done to make it easy to write unit tests, since the `Client` is a generic, all its dependencies (i.e. the server) can be mocked out. This also makes it a bit nicer to know if there is or isn't a client (via the server's client optional). Added a MemoryPool for the Send object (I thought that was a nice touch!) Removed MacOS hack on accept/conn completion usage. Known issues: - When framing an outgoing message, the entire message has to be duped. This is no worse than how it was before, but it should be possible to eliminate this in the future. Probably not part of this PR. - Websocket parsing will reject continuation frames. I don't know of a single client that will send a fragmented message (websocket has its own message fragmentation), but we should probably still support this just in case. - I don't think the receive, timeout and close completions can safely be re-used like we're doing. I believe they need to be associated with a specific client socket. - A new connection creates a new browser session. I think this is right (??), but for the very first, we're throwing out a perfectly usable session. I'm thinking this might be a change to how Browser/Sessions work. - zig build test won't compile. This branch reproduces the issue with none of these changes: https://github.com/karlseguin/browser/tree/broken_test_build (or, as a diff to main): lightpanda-io/browser@main...karlseguin:broken_test_build
Move more logic into the reader. Avoid copying partial messages in cases where we know that the buffer is large enough. This is mostly groundwork for trying to add support for continuation frames.
CDP is now an struct which contains its own state a browser and a session. When a client connection is made and successfully upgrades, the client creates the CDP instance. There is now a cleaner separation betwen Server, Client and CDP. Removed a number of allocations, especially when writing results/events from CDP to the client. Improved input message parsing. Tried to remove some usage of undefined.
|
Hi @karlseguin, first of all thanks for this PR, it's impressive. It seems that there is a regression (leak) on memory usage. When I launch 1000 runs of the puppeteer demo on my MBA M2, looking Activity Monitor I have:
|
In the page cdp, I was creating a copy of the page, thus copying the arena, which causes issues. I had removed There's still a leak on Mac from the cancel callback not being called and these allocations not being freed, but this predates the PR. |
Fixes issue where CDP closes the client, but client still registers a recv operation.
|
Great, that fixes the leak, thanks. I'm reviewing the code itself but it's a pretty big PR so it takes some time :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This includes / is built on top of #408
CDP is now a struct, with a cleaner separation between Server, Client and CDP. Server <-> Client <-> CDP -> Browser. The CDP instance is the state.
Optimized how cdp results and events are sent to the client - only a single allocation is now required.
Each CDP message is now processed with an arena, which is re-used for each message. Small ad-hoc message-specific allocations, like the
aux_datashould now be more efficient.Tried to remove some panics and undefines, or at least reduce the time that a field is undefined. Related, tried to remove the need for stable pointers (i.e.
Browser.initnow returns aBrowser, rather than needing to be given a*Browser).Improved parsing of CDP messages from the client.
Removed the newly added
browser.currentPage(), since the cdp already has directly access to the session. (browser.currentPage()would make it harder to implement the "TODO allow multiple sessions per browser.")Failed to get any unit test coverage from CDP, but i think it's important to do as there's more and more logic being added to it. Part of the blocker is global types. I'd consider introducing a global mock Browser/Session:
But I haven't given up hope on being able to quickly start a lightweight jsruntime.Env in unit tests.
To get a sense of the changes, I think you can look at
dom.discardSearchResultswhich shows:1 - how to get typed params
2 - how to access / modifity the state
3 - how to write a results