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

Use binary search for removing elements from Bag #878

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

nickoto
Copy link
Contributor

@nickoto nickoto commented Sep 14, 2023

Given that tokens are always added to the end of the array and have a monotonically increasing value, this list is always sorted, so we can use a binary search to improve performance if this list gets large.

Checklist

  • Updated CHANGELOG.md.

Given that tokens are always added to the end of the array and have a monotonically
increasing value, this list is always sorted, so we can use a binary search to improve
performance if this list gets large.
@mluisbrown
Copy link
Contributor

@nickoto did you run any tests to validate the performance improvement, and how significant it is? It would be useful to have some metrics.

@nickoto
Copy link
Contributor Author

nickoto commented Sep 15, 2023

Yes; in our particular scenario instruments was showing that for this particular operation in our app that Bag.remove was by far the hot path, taking several seconds to complete removing a large number of elements from the bag. After this change the removal process time went to several milliseconds in the exact same scenario. (In this case, the bag contained thousands of elements and the operation was removing half of them at once in no particular order).

While I can't test this over all workloads, in any situation where many items would be removed from the bag at once e.g. a large teardown, this would be a benefit. In situations where the bag is very tiny the complexity cost is negligible.

Copy link
Contributor

@mluisbrown mluisbrown left a comment

Choose a reason for hiding this comment

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

Looks like a good improvement 👍

@nickoto
Copy link
Contributor Author

nickoto commented Sep 27, 2023

@mluisbrown Since the runner for ubuntu 18.04 is deprecated we can no longer run the SwiftPM tests for 5.2 as there is no binary release of swift 5.2 on that platform (it only has releases for 16.04 and 18.04). That's why the log is showing a 404 - it fails to download a release that doesn't exist.

Not sure why the 5.7 runner was cancelled though -- it looks like it was passing up until it cancelled.

@mluisbrown
Copy link
Contributor

@nickoto We should drop the 5.2 tests then. I'm not sure if you have the rights to do that. If not let me know and I'll sort it out.

@nickoto
Copy link
Contributor Author

nickoto commented Sep 27, 2023

I can drop it from the yml but I'm not sure if there are other steps required.

Update: Yea I don't think I can get any further on my own, I don't even have permission to approve running the workflow @mluisbrown.

@mluisbrown mluisbrown merged commit 74dce06 into ReactiveCocoa:master Oct 11, 2023
5 checks passed
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