perf: avoid redundant CDP round-trips in evaluate and navigate#1341
perf: avoid redundant CDP round-trips in evaluate and navigate#1341pedro-saraiva88 wants to merge 3 commits into
Conversation
Read-only handlers that decorate their JSON response with the current page URL were issuing an extra `Runtime.evaluate location.href` after every operation, doubling the CDP round-trip count. Changes: 1. New `BrowserManager::cached_url()` and `cached_title()` accessors that read from `pages[active_page_index]`, which is already kept in sync by `navigate()` and Target events. Use these for response decoration. 2. `handle_evaluate` and 12 sibling read-only handlers (content, snapshot, gettext, getattribute, isvisible, isenabled, ischecked, ...) now call `mgr.cached_url()` instead of `mgr.get_url().await`. The `agent-browser url` user-facing handler keeps `get_url().await` so it stays accurate for SPAs that update via `history.pushState`. 3. `BrowserManager::navigate()` now fetches both `location.href` and `document.title` in a single `JSON.stringify([..., ...])` evaluate instead of two separate Runtime.evaluate calls. 4. `BrowserManager::navigate()` skips `wait_for_lifecycle` when the `Page.navigate` response carries no `loaderId` (same-document navigation, e.g. hash routing). Chrome does not fire `Page.loadEventFired` or `Page.domContentEventFired` in that case, so the previous code waited for an event that never arrived until the per-call timeout. Measured impact: - evaluate: 0.37ms -> 0.25ms (-32%) - navigate (full nav): 19.48ms -> 19.18ms (combined eval saves ~0.3ms) The remaining ~18ms gap to Playwright's raw `Page.navigate` (~0.74ms) is head-of-line blocking inside the CDP client's read loop when Chrome floods events during navigation. That fix is architectural (separating the response-routing path from the event broadcast path) and is left for a follow-up.
|
@pedro-saraiva88 is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
The cached PageInfo.url is updated by BrowserManager::navigate() and by Target events, but not by history.back(), history.forward(), Page.reload, or wait_for_url. Those four handlers were returning the pre-navigation URL via cached_url(), which is exactly the moment when the URL is most likely to have changed. Read live with mgr.get_url().await for these four handlers and add a short comment at each site explaining why the cache is bypassed. The nine remaining handlers that decorate read-only responses with origin keep cached_url() — they do not change the URL. Reported by Vercel VADE review on vercel-labs#1341.
|
Good catch — fixed in eff14ba.
The nine remaining handlers that decorate read-only responses with Validated with |
The JSON.stringify([location.href, document.title]) consolidation in BrowserManager::navigate() returned the correct values for the navigate response, but in some cases left the daemon's active session stuck on a different target — subsequent evaluate calls would run against about:blank or the Chrome debug index page (127.0.0.1:<cdpPort>) instead of the page just navigated to. Reproduced with `agent-browser open http://localhost:8080/admin/login` followed by `agent-browser eval "location.href"` returning the wrong URL. Restoring the two separate get_url() / get_title() round-trips fixes this without affecting the other measured wins: - evaluate -32% (cached_url() in 9 read-only handlers) is preserved - The wait_for_lifecycle skip on same-document navigations is preserved - The back/forward/reload/waitforurl live URL fix is preserved Net result on benchmarks: evaluate stays at -32%, navigate stays at 19.48ms (we keep the 0.3ms saving as future work, not worth the risk).
|
Self-reported regression: dropped the navigate eval consolidation in 429cbb5. While dogfooding this on a real Filament/Laravel app I found that the Updated PR contents:
Validated: |
1 participant
Summary
Read-only handlers that decorate their JSON response with the current page URL were issuing an extra
Runtime.evaluate location.hrefafter every operation, doubling the CDP round-trip count for ~13 commonly-used commands.BrowserManager::navigate()was also waiting on lifecycle events that Chrome never fires for same-document navigations.Measured impact
Benchmarked on a warm daemon, headless Chrome,
https://example.com, 5 runs each:evaluatenavigate(same-doc / hash routing)navigate(full)The headline win is
evaluatebecause it's in the hot path of every interaction loop (snapshot → ref → act).navigateis unchanged for full navigations — see "What's intentionally not in this PR" below.Changes
BrowserManager::cached_url()/cached_title()— read frompages[active_page_index], which is already kept in sync bynavigate()and Target events. No new state, no new sync paths.handle_evaluateand 8 sibling read-only handlers (handle_content,handle_snapshot,handle_gettext,handle_getattribute,handle_isvisible,handle_isenabled,handle_ischecked, ...) now callmgr.cached_url()instead ofmgr.get_url().await. Per the VADE review, four navigation-adjacent handlers (handle_back,handle_forward,handle_reload,handle_waitforurl) and the user-facinghandle_urlkeepget_url().awaitbecause they change the URL or are expected to return live state.BrowserManager::navigate()skipswait_for_lifecyclewhen thePage.navigateresponse carries noloaderId. That is Chrome's signal for a same-document navigation (e.g. hash routing). Without this guard the previous code would wait forPage.loadEventFired/Page.domContentEventFiredevents that Chrome never fires in that case, until the per-call timeout — which would surface as user-visible latency on SPA route changes done viaagent-browser open.Compatibility
cached_url()/cached_title()are additive helpers; nothing was removed from the public API.handle_url,handle_back,handle_forward,handle_reload,handle_waitforurlare deliberately unchanged behaviour — they always return the live URL, since the cache hasn't been refreshed yet at those points.loaderId.is_some()guard in navigate is purely defensive — same-doc navigations could not have been waiting on those events anyway, they were just timing out silently.Testing
End-to-end smoke against a local Filament/Laravel app:
agent-browser open http://localhost:8080/admin/loginfollowed byagent-browser eval "location.href"andagent-browser screenshot --full <path>— page renders, screenshot captures the actual login page, eval returns the navigated URL.Self-reported during this PR
While dogfooding this PR, an earlier revision tried to consolidate the URL+title fetch in
navigate()into a singleevaluate("JSON.stringify([...,...])")call to save one round-trip. That hunk left the daemon's active session pointing at a different target after navigation completed —evalwould run againstabout:blankor the Chrome debug index page (127.0.0.1:<cdpPort>). The two-round-trip URL+title fetch is restored in 429cbb5; bisected, isolated, and explained in the commit. The current diff has no perf claim that depends on that hunk.What's intentionally not in this PR
The remaining ~18 ms gap on
Page.navigateis head-of-line blocking in the CDP read loop: the reader processes events serially viaevent_tx.send()(atokio::broadcastchannel), and a slow subscriber can stall the reader until response delivery. Fixing that means either splitting the response-routing path from the event broadcast path or usingtry_sendon the broadcast — both warrant their own PR with dedicated benchmarks. Happy to follow up.