Skip to content
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
polm:fix-buffer-error-again
Jun 15, 2020
Merged

Don't explicitly release the memoryview in Grammar#127
sorami merged 1 commit intoWorksApplications:developfrom
polm:fix-buffer-error-again

Conversation

@polm
Copy link
Contributor

@polm polm commented Jun 14, 2020

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.

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.
@sorami sorami self-requested a review June 15, 2020 00:39
@sorami
Copy link
Collaborator

sorami commented Jun 15, 2020

@polm Thank you so much for this quick patch, and also a very detailed explanation.

@sorami sorami merged commit 1fa7774 into WorksApplications:develop Jun 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants