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

Simplify and speed up the SeekOffsets code #668

Merged
merged 2 commits into from
Jun 1, 2020

Conversation

ankon
Copy link
Contributor

@ankon ankon commented Mar 18, 2020

  • has: Use the similar function from the parent Map class, hoping that that
    is potentially faster than the brute-force approach through keys/some
  • pop: Replace the complex and inefficient O(n)-ish logic with a simpler
    O(1) approach.
    We don't need to make an array of all entries, reverse that, and then pop
    one element -- we only need to know the first key/value pair.

@ankon
Copy link
Contributor Author

ankon commented Mar 18, 2020

Hold off on this one please.

has doesn't work this way unfortunately, because of how the Map equality works. The change in pop should still be fine.

Replace the complex and inefficient O(n)-ish logic with a simpler
O(1) approach.
We don't need to make an array of all entries, reverse that, and then pop
one element -- we only need to know the first key/value pair.
@ankon
Copy link
Contributor Author

ankon commented Mar 19, 2020

Removed the has change; this should be good to go now.

I see the checks failing, and I have some issues locally on getting a stable test run -- looks a lot like flakes to me though (apart from the Azure database misconfiguration?)

@Nevon
Copy link
Collaborator

Nevon commented May 31, 2020

Could you merge latest master into this, and we can get it merged?

@ankon
Copy link
Contributor Author

ankon commented Jun 1, 2020

@Nevon sure :)

@Nevon Nevon merged commit 4b7e79b into tulios:master Jun 1, 2020
@ankon ankon deleted the pr/seek-simplify branch June 1, 2020 09:11
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