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.

Sign up for GitHub

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

Jump to bottom

Fix GC-safety issue with writev pointers in File. #2775

Merged
merged 1 commit into from
Jun 11, 2018
Merged

Fix GC-safety issue with writev pointers in File. #2775

merged 1 commit into from
Jun 11, 2018

Conversation

jemc
Copy link
Member

@jemc jemc commented Jun 10, 2018

Prior to this commit, there was a garbage-collection safety issue
with the way that the _pending_writev array worked in the File
class. The pending writev data was being stored in a flat array
with element type of USize, with the byte pointers being converted
to type USize in order to be stored alongside the buffer length
numbers in the same array.

Storing these pointers as numbers meant that they are no longer
traced by the garbage as being pointers, meaning that the garbage
collector would try to free the underlying memory due to no longer
being pointed to by any valid pointer.

This PR fixes the issue by using an Array[(Pointer[U8], USize)]
instead of an Array[USize]. This also eliminates some complexity
in the code that was related to accessing the array data in blocks
of 2 elements per logical element.

This resolves part of #2526.
We could resolve the issue entirely by doing the same fix for
TCPConnection, though that may have some addiitonal complexity from
needing to be compatible with IOCP on windows.

@jemc
Fix GC-safety issue with writev pointers in File.
3b50459 
Prior to this commit, there was a garbage-collection safety issue
with the way that the `_pending_writev` array worked in the `File`
class. The pending writev data was being stored in a flat array
with element type of `USize`, with the byte pointers being converted
to type `USize` in order to be stored alongside the buffer length
numbers in the same array.

Storing these pointers as numbers meant that they are no longer
traced by the garbage as being pointers, meaning that the garbage
collector would try to free the underlying memory due to no longer
being pointed to by any valid pointer.

This PR fixes the issue by using an `Array[(Pointer[U8], USize)]`
instead of an `Array[USize]`. This also eliminates some complexity
in the code that was related to accessing the array data in blocks
of 2 elements per logical element.

This resolves part of #2526.
We could resolve the issue entirely by doing the same fix for
TCPConnection, though that may have some addiitonal complexity from
needing to be compatible with IOCP on windows.
@jemc jemc added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Jun 10, 2018
@SeanTAllen
Copy link
Member

Nice @jemc

SeanTAllen
SeanTAllen reviewed Jun 11, 2018
View reviewed changes
packages/files/file.pony
@@ -478,28 +477,28 @@ class File
let writev_batch_size = @pony_os_writev_max()
while pending_total > 0 do
// Determine the number of bytes and buffers to send.
num_to_send = (_pending_writev.size().i32() / 2) - num_sent.i32()
num_to_send = _pending_writev.size().i32() - num_sent.i32()
Copy link
Member

Choose a reason for hiding this comment

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

Before I merge, can you explain the removal of /2 here and *2 etc elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, each io vector in the array took up two indices, one for the pointer itself and one for the size.

So, for example, if there were 4 io vectors, _pending_writev: Array[USize] would be 8 elements long, with indices 1, 3, 5, & 7 being pointers (converted to USize), and indices 2, 4, 6, & 8 being the associated sizes to those pointers.

After this change, if there are 4 io vectors, then _pending_writev: Array[(Pointer[U8] tag, USize)] will be 4 elements long, with each element being a tuple tuple containing both parts that were previously encoded at separate indices in the array.

@SeanTAllen
Copy link
Member

@jemc i left a question for you before merging. Mostly, I want to make sure I understand the change fully before hitting merge.

Re: TCPConnection comment, that was fixed a while back. Are you suggesting for "fully resolve" to switch to the tuple pattern here as a clean up to what is in TCPConnection? Or something else?

@jemc
Copy link
Member Author

jemc commented Jun 11, 2018 •
edited
Loading

Re: TCPConnection comment, that was fixed a while back. Are you suggesting for "fully resolve" to switch to the tuple pattern here as a clean up to what is in TCPConnection? Or something else?

Yes, the idea is to remove the _pending hack that adds unnecessary extra overhead to TCPConnection, by switching it to use this tuple pattern here. It will be a more clean solution than the current hack if we can make it work, and should theoretically improve performance as well (though maybe not noticeably).

@SeanTAllen SeanTAllen merged commit 8ee3410 into master Jun 11, 2018
@SeanTAllen SeanTAllen deleted the fix/file-pending-writev-gc branch June 11, 2018 10:57
ponylang-main added a commit that referenced this pull request Jun 11, 2018
@ponylang-main
Update CHANGELOG for PR #2775 [skip ci]
9a03821
@SeanTAllen
Copy link
Member

@jemc I'm closing #2526 as both bugs are closed. We can open an issue to clean up TCPConnection if you want.

@SeanTAllen SeanTAllen mentioned this pull request Jun 11, 2018
Closed
@jemc jemc mentioned this pull request Jun 11, 2018
Closed
@jemc
Copy link
Member Author

jemc commented Jun 11, 2018

Opened #2779.

@SeanTAllen SeanTAllen mentioned this pull request Jun 16, 2018
Closed
dipinhora added a commit to dipinhora/ponyc that referenced this pull request Dec 1, 2018
@dipinhora
Cleanup TCPConnection GC-safety mechanism for writev buffers
462cc9a 
Prior to this commit, the TCPConnection `pending_writev` buffer had to
have a secondary buffer called `_pending` to ensure that the data
wouldn't get GC'd too early by the runtime.

This commit removes that hack and replaces it with a slightly different
hack based on the strategy used in `File` in PR ponylang#2775. The hacky part
is that now we have two buffers for `writev` data. One for windows
(`_pending_windows`) and one for non-windows (`_pending_posix`) to
account for the difference in order of the struct between windows
and posix. This, however, seems less bad than the previous hack of
having the secondary buffer to ensure GC safety.

resolves ponylang#2782
resolves ponylang#2779
dipinhora added a commit to dipinhora/ponyc that referenced this pull request Jun 8, 2019
@dipinhora
Cleanup TCPConnection GC-safety mechanism for writev buffers
d5084d9 
Prior to this commit, the TCPConnection `pending_writev` buffer had to
have a secondary buffer called `_pending` to ensure that the data
wouldn't get GC'd too early by the runtime.

This commit removes that hack and replaces it with a slightly different
hack based on the strategy used in `File` in PR ponylang#2775. The hacky part
is that now we have two buffers for `writev` data. One for windows
(`_pending_windows`) and one for non-windows (`_pending_posix`) to
account for the difference in order of the struct between windows
and posix. This, however, seems less bad than the previous hack of
having the secondary buffer to ensure GC safety.

resolves ponylang#2782
resolves ponylang#2779
This was referenced Jun 8, 2019
Closed
Merged
dipinhora added a commit to dipinhora/ponyc that referenced this pull request Jun 9, 2019
@dipinhora
Cleanup TCPConnection GC-safety mechanism for writev buffers
32d6b51 
Prior to this commit, the TCPConnection `pending_writev` buffer had to
have a secondary buffer called `_pending` to ensure that the data
wouldn't get GC'd too early by the runtime.

This commit removes that hack and replaces it with a slightly different
hack based on the strategy used in `File` in PR ponylang#2775. The hacky part
is that now we have two buffers for `writev` data. One for windows
(`_pending_windows`) and one for non-windows (`_pending_posix`) to
account for the difference in order of the struct between windows
and posix. This, however, seems less bad than the previous hack of
having the secondary buffer to ensure GC safety.

resolves ponylang#2782
resolves ponylang#2779
SeanTAllen pushed a commit that referenced this pull request Jun 9, 2019
@dipinhora @SeanTAllen
Cleanup TCPConnection GC-safety mechanism for writev buffers
c48aa3c 
Prior to this commit, the TCPConnection `pending_writev` buffer had to
have a secondary buffer called `_pending` to ensure that the data
wouldn't get GC'd too early by the runtime.

This commit removes that hack and replaces it with a slightly different
hack based on the strategy used in `File` in PR #2775. The hacky part
is that now we have two buffers for `writev` data. One for windows
(`_pending_windows`) and one for non-windows (`_pending_posix`) to
account for the difference in order of the struct between windows
and posix. This, however, seems less bad than the previous hack of
having the secondary buffer to ensure GC safety.

resolves #2782
resolves #2779
patches11 pushed a commit to patches11/ponyc that referenced this pull request Jun 29, 2019
@dipinhora @patches11
Cleanup TCPConnection GC-safety mechanism for writev buffers
b1cd645 
Prior to this commit, the TCPConnection `pending_writev` buffer had to
have a secondary buffer called `_pending` to ensure that the data
wouldn't get GC'd too early by the runtime.

This commit removes that hack and replaces it with a slightly different
hack based on the strategy used in `File` in PR ponylang#2775. The hacky part
is that now we have two buffers for `writev` data. One for windows
(`_pending_windows`) and one for non-windows (`_pending_posix`) to
account for the difference in order of the struct between windows
and posix. This, however, seems less bad than the previous hack of
having the secondary buffer to ensure GC safety.

resolves ponylang#2782
resolves ponylang#2779
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SeanTAllen SeanTAllen SeanTAllen left review comments

@mfelsche mfelsche mfelsche approved these changes

Assignees
No one assigned
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jemc @SeanTAllen @mfelsche