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

Reduce array allocations by usage MemoryStream.GetBuffer() instead of ToArray() #1435

Merged
merged 2 commits into from
Oct 26, 2024

Conversation

lechu445
Copy link

No description provided.

@lechu445 lechu445 force-pushed the master branch 2 times, most recently from 89704bb to 6fbe46d Compare October 20, 2024 20:30
@tonyqus
Copy link
Member

tonyqus commented Oct 20, 2024

Check this document

Note that the buffer contains allocated bytes which might be unused. For example, if the string "test" is written into the MemoryStream object, the length of the buffer returned from GetBuffer is 256, not 4, with 252 bytes unused. To obtain only the data in the buffer, use the ToArray method; however, ToArray creates a copy of the data in
memory.

The return bytes of GetBuffer may not be the same as that from ToArray.

@tonyqus tonyqus closed this Oct 20, 2024
@lahma
Copy link
Collaborator

lahma commented Oct 21, 2024

I think in this PR the author is reading just the amount of Length so having "test" in the buffer and backing array being of size 256 is OK as only the elements 0-buffer.Length are being written?

@tonyqus tonyqus reopened this Oct 21, 2024
@tonyqus
Copy link
Member

tonyqus commented Oct 21, 2024

@lechu445 Please check if you can fix the following unit test

_/testcases/main/POIFS/FileSystem/TestNPOIFSFileSystem.cs#L724
System.UnauthorizedAccessException : MemoryStream's internal buffer cannot be accessed.

@tonyqus tonyqus modified the milestones: NPOI 2.7.3, NPOI 2.7.2 Oct 26, 2024
@tonyqus
Copy link
Member

tonyqus commented Oct 26, 2024

LGTM

@tonyqus tonyqus merged commit 0a0d59f into nissl-lab:master Oct 26, 2024
3 checks passed
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.

4 participants