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

only move the queue items if the new item index is less than size #498

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SMovaghati
Copy link

  • Does this PR have a descriptive title that could go in our release notes?
  • Does this PR add any new dependencies?
  • Does this PR modify any existing APIs?
  • Is the change to the API backwards compatible?
  • Should this result in any changes to our documentation, either updating existing docs or adding new ones?

What does this implement/fix? Briefly explain your changes.

Before, it used to always move the queue items, but that is not necessary if the item to add is at the end of the queue (and queue still has capacity).

@mmthomas
Copy link

mmthomas commented Jan 5, 2024

The code as is currenly correct; there is no rush to merge this change. However, it does allocate extra memory in the vector to support the current insertion strategy, and it keeps track of its own size and capacity.

We have a larger improvment to this whole class that simplifies the logic, reduces the memory requried, and removes the need for and explicit memmove, allowing std:vector to manage the insertion optimally.

@SMovaghati we should bring in all of our changes to this class instead of this small change.

Copy link

@mmthomas mmthomas left a comment

Choose a reason for hiding this comment

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

@SMovaghati I think we should bring our entire changes to this class for this PR; it simplifies the logic, it removes the extra size and capacity state, the need to allocate an extra item, and uses the built-in vector element moving when required.

@@ -78,7 +78,7 @@ class NeighborPriorityQueue
}
}

if (lo < _capacity)
if (lo < _size)

Choose a reason for hiding this comment

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

is there a check to make sure that _size + 1 <= _capacity? Otherwise we might run off the end of the vector.

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.

8 participants