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.

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

Re-implemented appendContent to persist a file handle and use Stream.IO.StreamWriter for writing. #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arizvisa
Copy link

@arizvisa arizvisa commented Jun 9, 2020

This is a preliminary reimplementation of the appendContent function inside winrmcp/cp.go. This aims to improve performance by reducing the system calls that Powershell will need to make when appending to a file.

When using an i/o redirect, Powershell will need to allocate a handle when opening a file, seek to its end, write the relevant data, and then close the handle. Another negative effect of redirections is that by default it will use UTF-16 which requires twice as many bytes as UTF-8. This PR fixes the deterimental effects of a i/o redirect by using UTF-8, and using the lower-level Stream.IO.StreamWriter class from the .NET framework. This will result in the file handle remaining kept open during the process of the writing of the base64 hunks so that less system calls will need to be made in the hopes that things will be a little faster.

I hate being the guy that uses channels for everything, but I wanted to keep the scope of the variable containing the Stream.IO.StreamWriter within a single function, and a channel seemed the best way to do this. :-/ I believe the same amount of data is being transferred over the socket, however the cost of file writing should be significantly reduced. Maybe it'll have an impact..

This is based on the second suggestion I made in PR #6. It also hasn't been tested too well, and I haven't tested its performance at all as it's just an implementation of a theory. It is also significantly more complex than PR #35 and thus more work could probably be done if you guys believe it should be considered.

@russianfool
Copy link

Is this repo all but dead? No commits for two years, hanging pull requests. packer-community in general seems to be low-activity compared to terraform and packer repos.

@arizvisa
Copy link
Author

Yeah. Kinda strange. :-/

@dylanmei
Copy link
Contributor

Speaking for myself, and specifically for this project which I was once passionate about, I no longer work with Windows and can't verify changes.

However, I believe the original team would agree with me that stewardship of this GitHub group could be opened up so that good things can still happen here, or else transfer projects to the right place. I'll reach out to them and post in a separate issue, linking back here. Once I've done that, the remainder of this thread can be about this particular PR.

@russianfool
Copy link

@dylanmei : No worries, appreciate the response and your contributions :) Didn't mean to derail the pull request, was just the "most-recently-active" thread with no open issue. I came here originally from hashicorp/packer#2648 (main issue for this, but closed there and I didn't want to necro) and this looked more promising than the parallelization in #6.

This may be a semi-moot point anyway as Windows Server 2019 supports OpenSSH. I've yet to test that + terraform/packer (and if remote-exec with ssh connector doesn't work due to command line stuff, local-exec scp should).

@dylanmei
Copy link
Contributor

FYI Seeking maintainers #38

@arizvisa
Copy link
Author

@dylanmei, you also should pin the issue. but don't forget announce it on hashicorp/packer, threatening removal even. packer's the major consumer of this library, so you're likely to find maintainers over there.

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