{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/17978","repository_url":"https://api.github.com/repos/dotnet/coreclr","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/17978/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/17978/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/17978/events","html_url":"https://github.com/dotnet/coreclr/pull/17978","id":322556886,"number":17978,"title":"Override Stream.CopyTo on FileStream","user":{"login":"tdinucci","id":35197667,"avatar_url":"https://avatars1.githubusercontent.com/u/35197667?v=4","gravatar_id":"","url":"https://api.github.com/users/tdinucci","html_url":"https://github.com/tdinucci","followers_url":"https://api.github.com/users/tdinucci/followers","following_url":"https://api.github.com/users/tdinucci/following{/other_user}","gists_url":"https://api.github.com/users/tdinucci/gists{/gist_id}","starred_url":"https://api.github.com/users/tdinucci/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tdinucci/subscriptions","organizations_url":"https://api.github.com/users/tdinucci/orgs","repos_url":"https://api.github.com/users/tdinucci/repos","events_url":"https://api.github.com/users/tdinucci/events{/privacy}","received_events_url":"https://api.github.com/users/tdinucci/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-05-13T01:55:38Z","updated_at":"2018-05-13T13:04:06Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/dotnet/coreclr/pulls/17978","html_url":"https://github.com/dotnet/coreclr/pull/17978","diff_url":"https://github.com/dotnet/coreclr/pull/17978.diff","patch_url":"https://github.com/dotnet/coreclr/pull/17978.patch"},"body":"Addresses https://github.com/dotnet/corefx/issues/29479\r\n\r\nThis is relevant to Windows only, just as CopyToAsync is.\r\n\r\nA PR for unit tests is about to be submitted to corefx.\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/388625356","html_url":"https://github.com/dotnet/coreclr/pull/17978#issuecomment-388625356","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/17978","id":388625356,"user":{"login":"stephentoub","id":2642209,"avatar_url":"https://avatars0.githubusercontent.com/u/2642209?v=4","gravatar_id":"","url":"https://api.github.com/users/stephentoub","html_url":"https://github.com/stephentoub","followers_url":"https://api.github.com/users/stephentoub/followers","following_url":"https://api.github.com/users/stephentoub/following{/other_user}","gists_url":"https://api.github.com/users/stephentoub/gists{/gist_id}","starred_url":"https://api.github.com/users/stephentoub/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stephentoub/subscriptions","organizations_url":"https://api.github.com/users/stephentoub/orgs","repos_url":"https://api.github.com/users/stephentoub/repos","events_url":"https://api.github.com/users/stephentoub/events{/privacy}","received_events_url":"https://api.github.com/users/stephentoub/received_events","type":"User","site_admin":false},"created_at":"2018-05-13T13:04:05Z","updated_at":"2018-05-13T13:04:05Z","author_association":"MEMBER","body":"I should have included more details on my thoughts in the issue I opened.\r\n\r\nMy specific thought was that at a minimum FileStream.CopyTo should delegate to CopyToAsync in the case where it's in async mode.  FileStream is a strange beast in that it's configured into one of two modes at construction time, either sync mode or async mode, the difference being whether it's created to use overlapped I/O on Windows, which needs to be specified at the time the handle is created.  Then all subsequent operations on the stream need to match, so if an async operation is performed on a sync stream, the implementation just queues a work item to the thread pool that calls the synchronous method, and conversely if a sync method is called on an async stream, it delegates to the XxAsync method and then blocks waiting for it to complete.  There is overhead associated with setting up each async operation, but there's even more overhead when a sync method is called on an async stream, as you not only get that per-operation async overhead, you also get the overhead of the blocking waiting for the result.  FileStream's CopyToAsync override is there to minimize the async overhead for each operation when in async mode.  With CopyTo not overridden, it just uses the base Stream's CopyTo, which will end up repeatedly calling Read and Write, each of which on an async FileStream will end up incurring the maximum overhead.  So at a minimum I was thinking we should do (pseudo-code):\r\n```C#\r\npublic override void CopyTo(Stream destination, int bufferSize)\r\n{\r\n    if (_asyncMode && GetType() == typeof(FileStream))\r\n    {\r\n        CopyToAsync(destination, bufferSize).GetAwaiter().GetResult();\r\n    }\r\n    else\r\n    {\r\n        base.CopyTo(destination, bufferSize);\r\n    }\r\n}\r\n```\r\nso that in the case where CopyTo is used on an async FileStream, we only incur the blocking and async overheads once rather than per read and write operation.\r\n\r\nIt's possible all of the additional duplication this PR provides also has benefits, but that's much less likely to be a given, and as Jan suggests would really need to be measured (as would my suggested change, but I'm more inclined to believe it'll have measurable impact)."}}
{"actor":{"display_login":"stephentoub"}}