This repository was archived by the owner on Mar 9, 2023. It is now read-only.
Don't explicitly release the memoryview in Grammar#127
Merged
sorami merged 1 commit intoWorksApplications:developfrom Jun 15, 2020
Merged
Don't explicitly release the memoryview in Grammar#127sorami merged 1 commit intoWorksApplications:developfrom
sorami merged 1 commit intoWorksApplications:developfrom
Conversation
This should fix the issue in WorksApplications#126 without sacrificing speed. The BufferError is caused by the Grammar calling release on its memoryview when it's deleted. With the error-causing code, that doesn't work because the Lattice has a reference to the memoryview, which is why the BufferError is thrown. The BufferError would represent a real problem if the Lattice was used after the Grammar was deleted, but that can't happen normally, and would throw all kinds of errors even with pre-Cython code. It seems natural for a class to release a memoryview it created, so I didn't think it was odd at first, but a memoryview is memory managed like any other Python object, so there's no need to manually release it unless you need to remove restrictions on the underlying data or something. It's fine to just let gc take care of it. In this case, manually releasing it causes issues because it's possible for the Grammar to be deleted before the Lattice. (I don't entirely follow why this only happens sometimes, but I think it has to do with the BinaryDictionary explicitly deleting the Grammar.) So now what happens is that the Grammar is created with a bytes object, creates a memoryview with a ref to the bytes object, and the Lattice creates a ref to the memoryview. Once the references to the Grammar and Lattice are removed (regardless of order), gc will take care of them appropriately.
Collaborator
|
@polm Thank you so much for this quick patch, and also a very detailed explanation. |
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This should fix the issue in #126 without sacrificing speed.
The BufferError is caused by the Grammar calling release on its
memoryview when it's deleted. With the error-causing code, that doesn't
work because the Lattice has a reference to the memoryview, which is why
the BufferError is thrown.
The BufferError would represent a real problem if the Lattice was used
after the Grammar was deleted, but that can't happen normally, and would
throw all kinds of errors even with pre-Cython code.
It seems natural for a class to release a memoryview it created, so I
didn't think it was odd at first, but a memoryview is memory managed
like any other Python object, so there's no need to manually release it
unless you need to remove restrictions on the underlying data or
something. It's fine to just let gc take care of it. In this case,
manually releasing it causes issues because it's possible for the
Grammar to be deleted before the Lattice. (I don't entirely follow why
this only happens sometimes, but I think it has to do with the
BinaryDictionary explicitly deleting the Grammar.)
So now what happens is that the Grammar is created with a bytes object,
creates a memoryview with a ref to the bytes object, and the Lattice
creates a ref to the memoryview. Once the references to the Grammar and
Lattice are removed (regardless of order), gc will take care of them
appropriately.