Persist command line strings to temporary storage to prevent excess in-memory use.#41955
Persist command line strings to temporary storage to prevent excess in-memory use.#419559 commits merged intodotnet:masterfrom
Conversation
| //if (stream.Length == 0) | ||
| //{ | ||
| // throw new ArgumentOutOfRangeException(); | ||
| //} |
There was a problem hiding this comment.
💭 Can we avoid writing to temporary storage if the string is empty?
There was a problem hiding this comment.
Good question! i'd prefer to avoid the extra complexity for what seems like a very small savings.
Are you ok with that?
| { | ||
| return; | ||
| } | ||
| _commandLineStorage.Dispose(); |
There was a problem hiding this comment.
On the first call from ctor, do we dispose the storage that we just allocated and allocate a new one? Would it make sense to keep _commandLineStorage uninitialized until this point?
There was a problem hiding this comment.
Also, creating a storage for an empty string seems unnecessary, so maybe the storage can be allocated only when the string becomes non-empty?
There was a problem hiding this comment.
I don't think we need to always avoid writing an empty string, but the initialization in the constructor could easily be removed. I'll let others speak for what they want though :)
| { | ||
| public static void WriteString(this ITemporaryStreamStorage storage, string value) | ||
| { | ||
| using var memoryStream = new MemoryStream(); |
There was a problem hiding this comment.
Can we use the MemoryStream(byte[]) constructor to avoid having to allocate our own StreamWriter?
new MemoryStream(Encoding.UTF8Encoding.GetBytes(value))
Not sure how much that saves us, but it seems desirable to allocate the correct size stream if we know it.
There was a problem hiding this comment.
i switched to a pooled approach so we can avoid some intermediary garbage. note, we still need the StreamWriter, but that's a tiny object (compared the byte[]s we save with pooling).
| var service = new TemporaryStorageServiceFactory.TemporaryStorageService(textFactory); | ||
| var storage = service.CreateTemporaryStreamStorage(CancellationToken.None); | ||
|
|
||
| // 0 length streams are allowed |
There was a problem hiding this comment.
nit: I would hope this comment is true given the name of the test :P
ryzngard
left a comment
There was a problem hiding this comment.
Mostly looks good. Would like to see us avoiding the initial storage allocation in the constructor and to investigate if we get savings using MemoryStream(byte[])
|
Feedback addressed. ptal again. Thanks! |
Fixes #37212