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

Fixed #641 - Increased Elements Collection performance #653

Merged
merged 2 commits into from Jan 3, 2018

Conversation

Projects
None yet
5 participants
@CaBocuk

CaBocuk commented Dec 24, 2017

Proposed changes

Is a proposal for #641 fix. The idea is in storing of initial state of elements collection and reducing amount of BySelectorCollection#getActualElements calls.
Now collection is being re-evaluated only in cases of element staleness or trying to call get(index) where index is out of current bounds.

As a result same test on amount of getActualElements() calls from #641 now shows only 2 collection re-evaluating instead of 365 for a collection of 182 elements.

Checklist

  • [+] Checkstyle and unit tests pass locally with my changes by running gradle check chrome htmlunit command
  • [-] I didn't provide any tests for performance but made sure that tests on staleness still work

Artsem_Savosik added some commits Dec 24, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 24, 2017

Coverage Status

Coverage increased (+0.04%) to 64.358% when pulling e79ed68 on CaBocuk:#641-Fix-of-elements-collection-performance into cfc83fd on codeborne:master.

coveralls commented Dec 24, 2017

Coverage Status

Coverage increased (+0.04%) to 64.358% when pulling e79ed68 on CaBocuk:#641-Fix-of-elements-collection-performance into cfc83fd on codeborne:master.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 24, 2017

Codecov Report

Merging #653 into master will increase coverage by 0.02%.
The diff coverage is 73.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #653      +/-   ##
============================================
+ Coverage     60.49%   60.51%   +0.02%     
  Complexity      771      771              
============================================
  Files           148      148              
  Lines          2749     2758       +9     
  Branches        270      270              
============================================
+ Hits           1663     1669       +6     
- Misses          981      983       +2     
- Partials        105      106       +1
Impacted Files Coverage Δ Complexity Δ
...ava/com/codeborne/selenide/ElementsCollection.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...rne/selenide/impl/SelenideElementListIterator.java 100% <100%> (ø) 8 <1> (ø) ⬇️
...com/codeborne/selenide/impl/CollectionElement.java 100% <100%> (ø) 7 <2> (ø) ⬇️
...deborne/selenide/impl/SelenideElementIterator.java 85.71% <66.66%> (-14.29%) 5 <3> (ø)

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 cfc83fd...e79ed68. Read the comment docs.

codecov-io commented Dec 24, 2017

Codecov Report

Merging #653 into master will increase coverage by 0.02%.
The diff coverage is 73.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #653      +/-   ##
============================================
+ Coverage     60.49%   60.51%   +0.02%     
  Complexity      771      771              
============================================
  Files           148      148              
  Lines          2749     2758       +9     
  Branches        270      270              
============================================
+ Hits           1663     1669       +6     
- Misses          981      983       +2     
- Partials        105      106       +1
Impacted Files Coverage Δ Complexity Δ
...ava/com/codeborne/selenide/ElementsCollection.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...rne/selenide/impl/SelenideElementListIterator.java 100% <100%> (ø) 8 <1> (ø) ⬇️
...com/codeborne/selenide/impl/CollectionElement.java 100% <100%> (ø) 7 <2> (ø) ⬇️
...deborne/selenide/impl/SelenideElementIterator.java 85.71% <66.66%> (-14.29%) 5 <3> (ø)

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 cfc83fd...e79ed68. Read the comment docs.

@CaBocuk CaBocuk changed the title from #641 Fix - Added storing initial state of elements collection to Fixed #641 - Increased Elements Collection performance Dec 24, 2017

@asolntsev asolntsev self-requested a review Dec 27, 2017

@asolntsev asolntsev self-assigned this Dec 27, 2017

@asolntsev asolntsev added this to the 4.10 milestone Dec 27, 2017

return el;
} catch (StaleElementReferenceException | IndexOutOfBoundsException e) {
actualElements = collection.getActualElements();
return actualElements.get(index);

This comment has been minimized.

@BorisOsipov

BorisOsipov Dec 28, 2017

Member

Sorry maybe I get something wrong.
In https://github.com/codeborne/selenide/pull/653/files#diff-7fb24574c58e442aadf59a781455f466R35 checks element for staleness, but here not. why not? is it correct assumption that isn't happening here again?

@BorisOsipov

BorisOsipov Dec 28, 2017

Member

Sorry maybe I get something wrong.
In https://github.com/codeborne/selenide/pull/653/files#diff-7fb24574c58e442aadf59a781455f466R35 checks element for staleness, but here not. why not? is it correct assumption that isn't happening here again?

This comment has been minimized.

@CaBocuk

CaBocuk Dec 28, 2017

Even if element becomes stale in catch block between actualElements re-evaluating and returning the element, the StaleElementRefferenceException will be caught by SelenideElementProxy. So then proxy will handle retry of calling some method on our element. So I mean that absence of staleness validation is quite okay in catch. It just means that collection is still in progress of loading. All timeout and retrying logics is business of SelenideElementProxy.

Just to explain why do we need this try-catch block.
Actual elements in try block may become stale because at this code-point we don't know what has passed this cached list to us and we don't know whether it is really actual or not.
But anyway we hope that list is actual -> and we try to get element that we need. Then we check whether it is stale or not. If not -> just return it.
But if element from cached list became stale or the index of the element is out of bounds -> it means that real collection in browser has been refreshed since this collection has been cached and we reach catch block because of one of two mentioned exceptions.
So staleness validation definitely should be present in try block. Because it is the only place where we can signalize that our collection of actual elements is stale.
So in catch block we just re-evaluate collection and try to return element that we get. We don't have to add any additional checks here because if smth is wrong -> SelenideElementProxy will do the retry for us.

Whew... I hope my explanation is rather clear 😃

@CaBocuk

CaBocuk Dec 28, 2017

Even if element becomes stale in catch block between actualElements re-evaluating and returning the element, the StaleElementRefferenceException will be caught by SelenideElementProxy. So then proxy will handle retry of calling some method on our element. So I mean that absence of staleness validation is quite okay in catch. It just means that collection is still in progress of loading. All timeout and retrying logics is business of SelenideElementProxy.

Just to explain why do we need this try-catch block.
Actual elements in try block may become stale because at this code-point we don't know what has passed this cached list to us and we don't know whether it is really actual or not.
But anyway we hope that list is actual -> and we try to get element that we need. Then we check whether it is stale or not. If not -> just return it.
But if element from cached list became stale or the index of the element is out of bounds -> it means that real collection in browser has been refreshed since this collection has been cached and we reach catch block because of one of two mentioned exceptions.
So staleness validation definitely should be present in try block. Because it is the only place where we can signalize that our collection of actual elements is stale.
So in catch block we just re-evaluate collection and try to return element that we get. We don't have to add any additional checks here because if smth is wrong -> SelenideElementProxy will do the retry for us.

Whew... I hope my explanation is rather clear 😃

This comment has been minimized.

@BorisOsipov

BorisOsipov Dec 28, 2017

Member

OK. I got you. Thank you for detailed explanation. I am not familiar with this code and such try\catch's looks strange for me😃

@BorisOsipov

BorisOsipov Dec 28, 2017

Member

OK. I got you. Thank you for detailed explanation. I am not familiar with this code and such try\catch's looks strange for me😃

return collection.getActualElements().get(index);
try {
WebElement el = actualElements.get(index);
el.isEnabled(); // check staleness

This comment has been minimized.

@asolntsev

asolntsev Jan 3, 2018

Contributor

It makes iteration slower. Every call to el.isEnabled() takes some time (because it sends an http request from webdriver to the browser).

But anyway, this solution is better than it was before. I think we can merge this PR and then think how to optimize this method further.

@asolntsev

asolntsev Jan 3, 2018

Contributor

It makes iteration slower. Every call to el.isEnabled() takes some time (because it sends an http request from webdriver to the browser).

But anyway, this solution is better than it was before. I think we can merge this PR and then think how to optimize this method further.

This comment has been minimized.

@CaBocuk

CaBocuk Jan 3, 2018

You're right. It makes it a bit slower. But I couldn't find any other ways to check element staleness without sending a request to browser. 😞

@CaBocuk

CaBocuk Jan 3, 2018

You're right. It makes it a bit slower. But I couldn't find any other ways to check element staleness without sending a request to browser. 😞

@asolntsev asolntsev merged commit d1aee69 into selenide:master Jan 3, 2018

4 checks passed

codecov/patch 73.33% of diff hit (target 60.49%)
Details
codecov/project 60.51% (+0.02%) compared to cfc83fd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 64.358%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment