Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance improvements on element creation #4940

Merged
merged 2 commits into from
Jan 13, 2020

Conversation

christian-bromann
Copy link
Member

Proposed changes

I found this while investigating some performance issues: when we fetch a large amount of elements we run some unnecessary computation within a loop which can be easily refactored. The flame chart changes from:

Screenshot 2020-01-03 at 23 50 02

to:

Screenshot 2020-01-04 at 00 52 21

which is a small but not insignificant win 🎉

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

@christian-bromann christian-bromann added Performance PR: Polish 💅 PRs that contain improvements on existing features backport-requested PRs with this label are considered to be applied to the last maintained version labels Jan 3, 2020
@christian-bromann christian-bromann requested a review from a team January 3, 2020 23:53
@codecov
Copy link

codecov bot commented Jan 4, 2020

Codecov Report

Merging #4940 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4940      +/-   ##
==========================================
+ Coverage   99.42%   99.42%   +<.01%     
==========================================
  Files         210      210              
  Lines        5535     5608      +73     
  Branches     1211     1240      +29     
==========================================
+ Hits         5503     5576      +73     
  Misses         29       29              
  Partials        3        3
Impacted Files Coverage Δ
packages/webdriverio/src/utils/getElementObject.js 97.87% <100%> (+0.04%) ⬆️
packages/webdriver/src/request.js 100% <0%> (ø) ⬆️
packages/wdio-runner/src/utils.js 100% <0%> (ø) ⬆️
packages/wdio-config/src/utils.js 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 808ba34...e1e3fee. Read the comment docs.

@gustavohenke
Copy link

gustavohenke commented Jan 4, 2020

which is a small but not insignificant win

I don't think it is a small one! The chart looks much cleaner now.

@christian-bromann
Copy link
Member Author

@erwinheitzman I adjusted both implementations, anything left?

@erwinheitzman
Copy link
Member

LGTM

@christian-bromann christian-bromann merged commit 83dbe5d into master Jan 13, 2020
@christian-bromann christian-bromann deleted the cb-perf-improvements branch January 13, 2020 12:56
@christian-bromann christian-bromann added backported PRs with this label got backported to the last maintained version and removed backport-requested PRs with this label are considered to be applied to the last maintained version labels Jan 17, 2020
christian-bromann added a commit that referenced this pull request Jan 17, 2020
* don't clone in the loop

* PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported PRs with this label got backported to the last maintained version Performance PR: Polish 💅 PRs that contain improvements on existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants