Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Avoid char buffer allocations in System.IO.Compression#2694

Merged
stephentoub merged 1 commit into
dotnet:masterfrom
jamesqo:comp
Aug 9, 2015
Merged

Avoid char buffer allocations in System.IO.Compression#2694
stephentoub merged 1 commit into
dotnet:masterfrom
jamesqo:comp

Conversation

@jamesqo
Copy link
Copy Markdown
Contributor

@jamesqo jamesqo commented Aug 9, 2015

These changes avoid creating an intermediate char[] buffer when creating a string.

@Maxwe11
Copy link
Copy Markdown
Contributor

Maxwe11 commented Aug 9, 2015

  1. your fork is out of date
  2. similar change (unsafe string mutation) was recently rejected (see comments in ZipFile: Reduce string allocations #2675)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As @Maxwe11 stated, we don't want to be doing String mutation like this, and @justinvp already changed this implementation in #2675. Please revert the changes to this file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Don't we do this all the time in String and StringBuilder for performance reasons, though? I realize that mundane tasks like this should avoid pointer arithmetic, but say if we had this in a performance-critical piece of code, then what are the guidelines?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't we do this all the time in String and StringBuilder for performance reasons, though?

Yes, but that's inside String, the same type, and StringBuilder, which is effectively an extension of that type. To code outside of such types, string should be considered immutable, except in very rare circumstances. The mutability is effectively an internal implementation detail.

but say if we had this in a performance-critical piece of code, then what are the guidelines?

The guideline is don't do it. If there's an extremely compelling case for doing it in a particular situation, complete with scenarios and perf tests and no alternatives, then it's possible an exception would be made.

@jamesqo
Copy link
Copy Markdown
Contributor Author

jamesqo commented Aug 9, 2015

@Maxwe11 I've updated the fork to resolve these problems.

@stephentoub
Copy link
Copy Markdown
Member

Thanks for updating it, @James-Ko. LGTM.

stephentoub added a commit that referenced this pull request Aug 9, 2015
Avoid char buffer allocations in System.IO.Compression
@stephentoub stephentoub merged commit 21bf9a8 into dotnet:master Aug 9, 2015
@jamesqo jamesqo deleted the comp branch August 9, 2015 19:01
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Avoid char buffer allocations in System.IO.Compression

Commit migrated from dotnet/corefx@21bf9a8
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants