Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

fix: optimize LRU cache #985

Merged
merged 8 commits into from
Feb 28, 2025
Merged

fix: optimize LRU cache #985

merged 8 commits into from
Feb 28, 2025
+55 −1

Conversation

abhinvv1
Copy link

@abhinvv1 abhinvv1 commented Feb 28, 2025 •
edited
Loading

What?

  • Currently, we simply call [element fb_takeSnapshot:NO] when checkStaleness was true.
  • But. if an element was stale and the snapshot method threw an exception (which indicates the element is no longer valid), that exception would propagate upward.
  • Issue: The stale element remained in the cache, meaning that it continued to occupy memory and could be repeatedly accessed or retried, leading to a memory spike over time.
  • This PR is to add removeObjectForKey: method to remove an object from the LRU cache using its key.
  • If [element fb_takeSnapshot:NO] throws an exception (implying the element is stale), the catch block is executed.
  • In the catch block:
    • The stale element is explicitly removed from the cache using a synchronized block.
    • The exception is then re-thrown so that the error handling remains consistent for the calling code.

Impact:

  • Performance remains O(1) for removals i.e. minimal overhead

mykola-mokhnach
mykola-mokhnach reviewed Feb 28, 2025
View reviewed changes
WebDriverAgentLib/Routing/FBElementCache.m
@@ -73,7 +73,13 @@ - (XCUIElement *)elementForUUID:(NSString *)uuid checkStaleness:(BOOL)checkStale
@throw [NSException exceptionWithName:FBStaleElementException reason:reason userInfo:@{}];
}
if (checkStaleness) {
Copy link

Choose a reason for hiding this comment

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

would it be possible to add an integration test to validate this behaviour?

Copy link
Author

Choose a reason for hiding this comment

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

can we pick this up later? is it a release blocker? to how much extent do we cover this in tests currently?

Copy link

Choose a reason for hiding this comment

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

yes, it is ok to do it in a different PR

@abhinvv1 abhinvv1 requested a review from KazuCocoa February 28, 2025 11:51
@mykola-mokhnach mykola-mokhnach merged commit 46dc417 into appium:master Feb 28, 2025
41 of 43 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 28, 2025
@semantic-release-bot
chore(release): 9.0.6 [skip ci]
d7cff1a 
## [9.0.6](v9.0.5...v9.0.6) (2025-02-28)

### Bug Fixes

* optimize LRU cache ([#985](#985)) ([46dc417](46dc417))
@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 9.0.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@KazuCocoa KazuCocoa KazuCocoa approved these changes

@mykola-mokhnach mykola-mokhnach mykola-mokhnach approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abhinvv1 @KazuCocoa @mykola-mokhnach