Skip to content

Don't dealloc strings if the TensorBuffer is shared (#357) - #358

Merged
karllessard merged 2 commits into
tensorflow:masterfrom
brychcy:master
Aug 6, 2021
Merged

karllessard merged 2 commits into
tensorflow:masterfrom
brychcy:master

Conversation

@brychcy

@brychcy brychcy commented Jul 30, 2021

Copy link
Copy Markdown
Contributor

No description provided.

@saudet

saudet commented Aug 4, 2021

Copy link
Copy Markdown
Contributor

There's no more straightforward way to get the information? It doesn't sound like TF_TensorMaybeMove() is always going to return null when the content is being shared.

@brychcy

brychcy commented Aug 4, 2021

Copy link
Copy Markdown
Contributor Author

There's no more straightforward way to get the information? It doesn't sound like TF_TensorMaybeMove() is always going to return null when the content is being shared.

If there is, I haven't found it.

BTW, what I really wonder is: Shouldn't string deallocation be done automatically on the native Tensorflow level when the last tensor that uses the TensorBuffer is released?

@saudet

saudet commented Aug 5, 2021

Copy link
Copy Markdown
Contributor

No, that doesn't happen. We need to allocate the strings manually, and deallocate them manually or we end up with memory leaks, see issue #251.

karllessard
karllessard previously approved these changes Aug 5, 2021

@karllessard karllessard left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Even if we are not 100% sure that this fix will cover all cases, it seemed to work for @brychcy so let's merge it to give it a try and see. Thanks @brychcy !

@karllessard

Copy link
Copy Markdown
Collaborator

Oops, it looks like the modified code is not formatted correctly, @brychcy can you please run mvn spotless:apply locally before pushing a newer version of this PR?

@brychcy

brychcy commented Aug 5, 2021 •

Copy link
Copy Markdown
Contributor Author

Oops, it looks like the modified code is not formatted correctly, @brychcy can you please run mvn spotless:apply locally before pushing a newer version of this PR?

Done.

At first it didn't do anything for me because origin/master pointed to my version and
<ratchetFrom>origin/master</ratchetFrom>
is used.

As the whole file was reformatted, I have done it in a separate commit.

@karllessard karllessard left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot @brychcy for fixing this!

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.

3 participants