Track patients with unordered_set rather than vector #1253
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The stored vector of pybind-registered-type patients can grow without bound if a function is called with the same patient multiple times. This commit changes the pybind internal patient storage to an `unordered_set` to avoid the issue. Fixes pybind#1251
Merged
19 tasks
|
Hi @jagerman -- sorry for not getting to this (now-ancient PR) earlier. This is just to say that your change looks good to me. We could merge it to master and consider it for inclusion to the next minor version. Would you be able to address the conflicts? |
…patient-constraint
|
I'm briefly trying to see if I can update this PR, resolving merge conflicts, per discussion in #1251. (For now, will do fast-forward merge) |
- Checks `not m.has_patients()` after nurse is gc'd - Uses Python implementation of `refcount`
EricCousineau-TRI
pushed a commit
to EricCousineau-TRI/pybind11
that referenced
this pull request
Jul 10, 2020
This fixes pybind#1251 (patient vector grows without bounds) for the 2.2.2 branch by checking that the vector doesn't already have the given patient. This is a little less elegant than the same fix for `master` in pybind#1253 (which changes the patients `vector` to an `unordered_set`), but that requires an internals layout change, which this approach avoids.
|
Filed #2289 per @YannickJadoul's request: My pref is on this PR, if we think it'll be more efficient. |
|
Same thing as before: depends on whether/when we want to break the internal ABI ;-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The stored vector of pybind-registered-type patients can grow without
bound if a function is called with the same patient multiple times.
This commit changes the pybind internal patient storage to an
unordered_setto avoid the issue.Fixes #1251
I've also pushed a separate fix for 2.2.2 (in #1250) that solves this by walking through the vector to avoid storing a duplicate. (It's not as nice a fix, but it doesn't require changing the
internalsstructure for 2.2.2).EDIT(eric): #2289 is an alternative or precursor to this PR.