Skip to content

Comments

fix: performance#658

Merged
Siolto merged 19 commits intov9-supportfrom
v9_performance_fix_0.1
Jun 4, 2025
Merged

fix: performance#658
Siolto merged 19 commits intov9-supportfrom
v9_performance_fix_0.1

Conversation

@Siolto
Copy link
Collaborator

@Siolto Siolto commented Feb 11, 2025

First performance improvement of the v9 support.

current improvement for the basic.test.js from ~33 sec to ~13 sec. No further performance tests happened yet.

it was a late night session so the description will get updated. But not yet...

@Siolto
Copy link
Collaborator Author

Siolto commented Feb 12, 2025

I call it the Promisified callback hell 🔥 🔥

As far as I can tell the performance issues are a combination of our own waitForUI5, which is not fully promisified and still uses callbacks and the use of these callbacks in executeControlMethod and executeObjectMethod.

Current solution idea:

  • DON'T use any callbacks in waitForUI5 => for the time being, use the RecordReplay.waitForUI5
  • check if we can get rid of the callbacks in our own waitForUI5 function or even if we still need it for performance reasons.
  • I am not sure if this will solve all our performance issues but at least for me it makes it a lot faster

Ping: @vobu @marianfoo @hmanchev @monavari-lebrecht @nicoschoenteich

@Siolto Siolto mentioned this pull request Feb 12, 2025
@Siolto
Copy link
Collaborator Author

Siolto commented Feb 12, 2025

Performance update

I had to slow down BIDI due to fragile tests (Promise.all is deadly 💀). This comparison is not 100% accurate as I had to fix some tests on the way but we can see the improvements.

There are some BIDI related topics we have to discuss (marked with TODO in the PR) before merging this.

old BIDI performance:

image

new BIDI performance:

Bildschirmfoto 2025-02-12 um 20 52 01

old webdriver performance:

image

new webdriver performance:

image

@vobu
Copy link
Contributor

vobu commented Feb 13, 2025

you clearly are on a spree here and got further than any of us before. Kudos 🙇
Are the old..new comparisons relating to wdi5^2 vs wdi5^3 or is this wdi5^3-rc.0 vs the current codebase?

@Siolto
Copy link
Collaborator Author

Siolto commented Feb 24, 2025

you clearly are on a spree here and got further than any of us before. Kudos 🙇 Are the old..new comparisons relating to wdi5^2 vs wdi5^3 or is this wdi5^3-rc.0 vs the current codebase?

That was wdi5^3-rc.0 vs current codebase. For a fair comparison wdi5^2 vs current codebase you have to align the number of maximum instances since the default has increased with v3. If you want you can do this missing comparison 😉

My assumption is that we are still around 10-20% slower with the current codebase, but I am optimistic that we can reduce this even more.

@Siolto Siolto marked this pull request as ready for review April 10, 2025 18:35
@Siolto
Copy link
Collaborator Author

Siolto commented Apr 10, 2025

Removed our own waitForUI5 and use the RecordReplay.waitForUI5. I think this first improvement is good to go to get some things moving with wdi5^3.x

@vobu
Copy link
Contributor

vobu commented May 20, 2025

thanks to the massive work of @Siolto, we're now seeing a ~20% performance increase in this PR 🚤 🥳

subject: 3 testruns of 25 test suites

wdi5^2: (average of) 1:36 min (WebdriverIO 8, webdriver protocol)
wdi5^3: (average of) 1:16 min (WebdriverIO 9, both with webdriver and bidi)

// compare results
regularPeopleListNames.forEach((name) => {
expect(peopleListNames).toContain(name)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Siolto
browser.execute is aimed at synchronous callbacks
but can handle async ones
however: if Promise.all is used, and all Promises are unwrapped inside out
we have https://github.com/webdriverio/webdriverio/blob/b70285e4599408da13479ebef0fb95a8cb13d60f/packages/webdriverio/src/commands/browser/execute.ts#L61 resolving
the node <- wdi5 -> browser communication prematurely, resulting in "undefined" results

@Siolto Siolto merged commit d63818d into v9-support Jun 4, 2025
1 of 2 checks passed
mauriciolauffer pushed a commit to mauriciolauffer/wdi5 that referenced this pull request Jun 6, 2025
* fix: performance

* fix: improve error handling and result processing in executeControlMethod

* fix: refactor executeObjectMethod not using callbacks

* test: update DateTimePicker API test to use ISO string format

* fix: update generated-methods test to handle different date formats based on protocol

* fix: await browser.asControl in press test for proper asynchronous handling

* fix: bidi race condition in retrieveElements

* fix: update Planning Calendar test to handle BIDI-specific DOM controls

* fix: optimize control retrieval for BIDI by removing unnecessary await

* fix: adjust BIDI handling in exec test to avoid Promise.all for performance

* chore: configuration

* fix: add TODO comment for handling UI5 injection options

* refactor: remove propietary waitForUI5

* refactor: run planningCalendar test also with BiDi protocol

* fix: improve peformance while retrieving multiple elements (ui5-community#665)

[skip ci]

* refactor: rename clientSide__checkForUI5Ready to clientSide_checkForUI5Ready for consistency

* chore: lock file

* fix: test

* refactor: rm unnecessary await

---------

Co-authored-by: Volker Buzek <dev@metalodge.com>
vobu added a commit that referenced this pull request Jul 6, 2025
* ci: Improve ESLint v9 config

* ci: Bump GH Actions checkout and setup-node to v4

* ci: Bump GH Actions github/codeql-action/*  to v3

* ci: Bump GH Actions download-artifact and upload-artifact to v4

* ci: Bump prettier version

* chore: Bump types versions

* chore: Linting + types

* fix: Mostly any types

* fix: Remove deprecated browser.switchToFrame

* fix: use crypto.randomUUID

* refactor: load UI5 libs in 1 task

* fix: remove multiple any types

* chore: linting

* fix: performance (#658)

* fix: performance

* fix: improve error handling and result processing in executeControlMethod

* fix: refactor executeObjectMethod not using callbacks

* test: update DateTimePicker API test to use ISO string format

* fix: update generated-methods test to handle different date formats based on protocol

* fix: await browser.asControl in press test for proper asynchronous handling

* fix: bidi race condition in retrieveElements

* fix: update Planning Calendar test to handle BIDI-specific DOM controls

* fix: optimize control retrieval for BIDI by removing unnecessary await

* fix: adjust BIDI handling in exec test to avoid Promise.all for performance

* chore: configuration

* fix: add TODO comment for handling UI5 injection options

* refactor: remove propietary waitForUI5

* refactor: run planningCalendar test also with BiDi protocol

* fix: improve peformance while retrieving multiple elements (#665)

[skip ci]

* refactor: rename clientSide__checkForUI5Ready to clientSide_checkForUI5Ready for consistency

* chore: lock file

* fix: test

* refactor: rm unnecessary await

---------

Co-authored-by: Volker Buzek <dev@metalodge.com>

* fix: use crypto.randomUUID

* fix: revert

* build: clean script

* test: add more UI5 LTS versions

* refactor: remove fs-extra

* chore: pretty sure package "i" was installed by mistake

* test: fix expected value

* chore: update workflows to also run against v9-support

* test: use a public method to test selected element

* test: expect().toHaveTextContaining() doesn't exist

* test: replace getInitStatus() by isInitialized()

* test: fix wdio-ui5tooling.conf.js

* test: remove deepmerge dependency

* chore: npm shouldn't be a dependency

* test: simplify test deps with peerDeps

* test: replace sinon by node:test

* fix: remove devtools dep

* fix: use browserInstance rather than global browser

* fix: no console - use wdi5 Logger

* refactor: compare-versions package as a real dependency

* refactor: remove more browser globals

* test: add browserVersion to make tests more stable + no node:join in specs

* chore: prettier

* chore: remove old eslint files

* fix: baseUrl is mandatory, must fail if missing

* fix: load ui5 libs at once

* refactor: client-side-js is also typed

* fix: use control.getMetadata().getName()

* docs: add statement for new bidi protocol

* Update docs/migration.md

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

* fix: multiremote race condition where injectUI5 was not done

* wip: use stable ubuntu version

* wip: decrease ubuntu version

* chore: add multiremote to the tests again

* refactor: linting

* refactor: ts erasableSyntaxOnly

* fix: wdio bridge

* chore: use stable ubuntu version due to chrome138 pipeline issues

* fix: increase timeout for ts tests in pipeline

---------

Co-authored-by: Simon Coen <32054457+Siolto@users.noreply.github.com>
Co-authored-by: Volker Buzek <dev@metalodge.com>
Co-authored-by: Marian Zeis <13335743+marianfoo@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: Siolto <simon.coen@js-soft.com>
@vobu vobu deleted the v9_performance_fix_0.1 branch July 7, 2025 10:03
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