Skip to content

perf: memoize the browser's CDP connection - #61

Merged
PrestaEdit merged 1 commit into
PrestaFlow:mainfrom
hugo-fasone:memoisation-connexion
Aug 31, 2026
Merged

perf: memoize the browser's CDP connection#61
PrestaEdit merged 1 commit into
PrestaFlow:mainfrom
hugo-fasone:memoisation-connexion

Conversation

@hugo-fasone

Copy link
Copy Markdown
Contributor

Context

On any suite that calls importPage(), runtime is dominated by redundant CDP
reconnections rather than by actual browser work. Measured on a project with 2 suites /
5 tests (PHP 8.3, headless Chromium in a container, PS 9.1 target): 23.67 s, of
which roughly 15 s was pure bootstrap.

Launching Chromium accounts for only 208 ms — the cost is elsewhere.

Root cause

TestsSuite::getBrowser() re-reads the socket file and issues a fresh
BrowserFactory::connectToBrowser() on every call, never reusing the instance. Two
effects compound:

Operation Measured cost
connectToBrowser() 101 ms, constant
getPages() 205 → 506 → 406 → 660 ms (growing: every connection left open makes target enumeration heavier)
TestsSuite::getBrowser() ~102 ms
TestsSuite::getPage() ~1,250 ms (2 × getBrowser())

The multiplier lives in CommonPage::__call(), which evaluates getPage() three
times
— condition, method_exists, invocation:

if (!is_null($this->getPage()) && method_exists($this->getPage(), $name)) {
    call_user_func_array([$this->getPage(), $name], $arguments);
}

And importPage() calls $pageInstance->setUserAgent(...), which does not exist on the
library's Page classes and therefore goes through that __call. Full chain:

importPage()  →  setUserAgent()  →  __call  →  3 × getPage()  →  6 × connectToBrowser
              ≈ 3 × 1,250 ms  =  3.7 s per imported page

A suite importing 3 pages thus paid ~11 s before running its first test.

Changes

1. TestsSuite::getBrowser() — reuse the connected instance

Two statics ($browserInstance, $browserInstanceSocket) plus a guard at the top of the
method. Invalidation has three conditions, so a dead browser is never handed back:

  • the socket URI is gone (end of run, @unlink by ExecuteSuite);
  • the URI differs from the memoized one (browser relaunched elsewhere);
  • getConnection()->isConnected() is false (browser closed or crashed).

isConnected() performs no CDP round-trip — it only reads the local socket state in
wrench\Client — so the guard stays free. ExecuteSuite::getBrowser(force: false)?->close()
still retrieves the live instance to close it at the end of the run.

2. TestsSuite::getPage() — one enumeration per attempt

getBrowser() is hoisted into a variable, and getPages() is only re-issued after an
actual createPage() (the first list is otherwise already current). The ×3 retry loop and
the applyExtraHttpHeaders() semantics are unchanged.

3. CommonPage::__call() — evaluate getPage() once

Results

Before After
getBrowser() (already connected) 102 ms, constant 0 ms
getPage() ~1,250 ms 166 ms, then 0 ms
init() = 1 × importPage() 3,910 ms 3 ms
Smoke suite (3 tests) 10.53 s 3.37 s
Full run (2 suites, 5 tests) 23.67 s 3.60 s

Compatibility

No API change and no observable behaviour change:

  • __call() still returns nothing — I deliberately did not add a return on
    call_user_func_array, since that would change the return value of every proxied call
    and is out of scope for a performance fix;
  • getPage() keeps its return $pages[0] (same semantics on an empty list) and its retry
    loop;
  • a dead browser now raises an error instead of being silently relaunched mid-suite. This
    is the more honest behaviour: the previous silent relaunch dropped the session and
    cookies, producing failures that were hard to interpret.

Testing

  • vendor/bin/phpunit --testsuite Unit: OK (117 tests, 442 assertions) on PHP 8.3.
  • Integration run against a real PS 9.1 shop: 5 tests, same results as before the patch.
  • The full importPage() path (3 pages, with BO credentials) exercised separately: imports
    succeed and the resulting failure is the login assertion, as expected.

@PrestaEdit

PrestaEdit commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Huge thanks @hugo-fasone for this PR 🙏

The analysis is genuinely outstanding: root cause traced all the way down to the triple getPage() inside __call, measurements at every step (connectToBrowser 101 ms, getPages growing as stale connections accumulate, importPage blowing up to 3.9 s…), and a before/after table that speaks for itself — 23.67 s → 3.60 s on a full run, a massive win for every user of the library.

The fix is surgical and minimal:

  • three-condition invalidation in getBrowser() (socket gone / socket changed / isConnected() false),
  • a single getBrowser() + a single enumeration per attempt in getPage(),
  • one evaluation of getPage() in __call.

No API change, and the behaviour shift is spelled out in the "Compatibility" section (a dead browser now raising an error instead of being silently relaunched is indeed the more honest behaviour).

LGTM, merging. Thanks again for the quality of the contribution 👏

@PrestaEdit
PrestaEdit merged commit 5975c70 into PrestaFlow:main Aug 31, 2026
PrestaEdit added a commit that referenced this pull request Aug 31, 2026
…thods

Suite au cache browser de #61, chaque getPage() reste peu coûteux mais reste
un appel Chrome target enumeration. Les méthodes qui invoquaient getPage()
plusieurs fois dans le même bloc paient encore ce prix inutilement.

Factorise $page = $this->getPage() en tête de méthode dans :
- CommonPage : visualCheckpoint, getTextContent, getInputValue, navigateTo,
  click, setValue
- BackOfficePage : goToSubMenu
- Products/Page : goToNewProduct
- OrderView/Page : openShippingModal

Aucun changement de sémantique.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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