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

Presize MemoryStream when possible. #14529

Merged
merged 1 commit into from Dec 18, 2017

Conversation

RoosterDragon
Copy link
Member

Also use GetBuffer when we know we have presized the stream to the exact required size to prevent a needless copy.

Helps with #14178 by reducing allocations during loading (the main culprits are ZipFile and MixFile), decreasing the peak memory reached when loading. The other stuff is mostly just along for the ride. :)

@@ -240,7 +240,7 @@ public byte[] Serialize()
{
if (IsImmediate)
{
var ret = new MemoryStream();
var ret = new MemoryStream(1 + OrderString.Length + TargetString.Length);
Copy link
Member

@pchote pchote Dec 16, 2017

Choose a reason for hiding this comment

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

w.Write(string) includes a length prefix so this is always at least two too few bytes, but possibly more if the strings are long!

Copy link
Member Author

Choose a reason for hiding this comment

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

Being wrong isn't the end of the world, the stream can still resize. This just gets us in the right ballpark. Alternatively, I could remove it if it's misleading.

Copy link
Member

Choose a reason for hiding this comment

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

How about we change this to 3 + OrderString.Length + TargetString.Length? That will be correct so long as both strings are less than 255 characters, which should be the most common case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Spelt it 1 + OrderString.Length 1 + TargetString.Length + 1 so it ties up with the things we're writing out like the other call-sites.

@@ -49,7 +49,7 @@ public static ServerOrder Deserialize(BinaryReader r)

public byte[] Serialize()
{
var ms = new MemoryStream();
var ms = new MemoryStream(1 + Name.Length + Data.Length);
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here.

@pchote pchote added this to the Next release milestone Dec 16, 2017
Also use GetBuffer when we know we have presized the stream to the exact required size to prevent a needless copy.
@abcdefg30 abcdefg30 merged commit ca01a1f into OpenRA:bleed Dec 18, 2017
@abcdefg30
Copy link
Member

Changelog

@RoosterDragon RoosterDragon deleted the memory-stream-presize branch December 19, 2017 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants