[Testing] tweaks to data reading in Ole files - #420
Numpsy wants to merge 6 commits into
Conversation
Sorry, something went wrong.
| <TargetFrameworks Condition="$([MSBuild]::IsOSPlatform('windows')) == false">net10.0</TargetFrameworks> | ||
| <TargetFrameworks Condition="$([MSBuild]::IsOSPlatform('windows'))">net48;net10.0-windows</TargetFrameworks> | ||
| <TargetFrameworks Condition="$([MSBuild]::IsOSPlatform('windows')) == false">net10.0;net8.0</TargetFrameworks> | ||
| <TargetFrameworks Condition="$([MSBuild]::IsOSPlatform('windows'))">net48;net10.0-windows;net8.0-windows</TargetFrameworks> |
There was a problem hiding this comment.
Just added .NET 8 for testing because BinaryReader.ReadExactly isn't built into .NET 8 like the Stream equivalent is
Sorry, something went wrong.
| #if !NET | ||
| private static int Read(this Stream target, Span<byte> buffer) | ||
| { | ||
| var sharedBuffer = ArrayPool<byte>.Shared.Rent(buffer.Length); |
There was a problem hiding this comment.
Borrowed from PollyFill to get things building on .NET Standard
Sorry, something went wrong.
|
Before: | Method | Mean | Error | StdDev | Gen0 | Allocated | |----------------------------------------- |---------:|----------:|----------:|-------:|----------:| | ReadSummaryInformation | 2.632 us | 0.0759 us | 0.0042 us | 0.1678 | 2.79 KB | | ReadDocumentSummaryInformation | 5.589 us | 0.5313 us | 0.0291 us | 0.2899 | 4.8 KB | | ReadWinUnicodeDocumentSummaryInformation | 4.634 us | 0.3511 us | 0.0192 us | 0.3510 | 5.73 KB | After: | Method | Mean | Error | StdDev | Gen0 | Allocated | |----------------------------------------- |---------:|----------:|----------:|-------:|----------:| | ReadSummaryInformation | 2.598 us | 0.0834 us | 0.0046 us | 0.1526 | 2.55 KB | | ReadDocumentSummaryInformation | 4.123 us | 0.0845 us | 0.0046 us | 0.2441 | 4.11 KB | | ReadWinUnicodeDocumentSummaryInformation | 4.073 us | 0.0664 us | 0.0036 us | 0.2823 | 4.7 KB | |
Sorry, something went wrong.
|
|
||
| // Read a null terminated string from the specified BinaryReader, using the specified length and codepage | ||
| // Note: Encoding.GetEncoding seems to actually be quite slow, so allow callers that already have the Encoding to provide it directly. | ||
| public static string ReadNullTerminatedStringWithEncoding(this BinaryReader target, int byteLength, int codePage, Encoding encoding) |
There was a problem hiding this comment.
int codePage is a redundant parameter since it's specified by Encoding - it's not validated and so prone to usage error.
Sorry, something went wrong.
| byte[] nameBytes = new byte[byteLength]; | ||
| target.ReadExactly(nameBytes.AsSpan()); | ||
| #else | ||
| // @@TBD@@ What max stack alloc size should be used here? |
There was a problem hiding this comment.
My understanding is that stackalloc up to 1024 bytes is generally considered to be OK
Sorry, something went wrong.
| #endif | ||
| } | ||
|
|
||
| public static string ReadNullTerminatedWideString(this BinaryReader target, int characterLength) => target.ReadNullTerminatedStringWithEncoding(byteLength: characterLength * 2, CodePages.WinUnicode, Encoding.Unicode); |
There was a problem hiding this comment.
There are more encodings that are wide than just WinUnicode (even if they're technically not allowed by OLE). The API here is also open to misuse wrt byte vs char vs code point length (you only know from the parameter name). Probably better if there was just one method that took the encoding along with the code point length or better still just read the length itself.
Sorry, something went wrong.
| // @@TODO@@ Is it worth special caseing UTF-8 as well? | ||
| internal static Encoding GetEncodingForCodePage(int codePage) | ||
| { | ||
| return codePage switch |
There was a problem hiding this comment.
Special-casing common code pages for performance is OK, but the greater win here would be to introduce additional validation. There are code pages which will give invalid results from the encoder for OLE rather than throw an error.
Sorry, something went wrong.
|
I'll get back to this next week, the HWP related stuff is something I could do with sorting first. |
Sorry, something went wrong.
Just a couple of places for testing, could be done elsewhere - - Use ReadExactly for reading dictionary strings and padding - Use ReadExactly with stackalloc for skipping padding, to remove some allocations Most of the complexity is pollyfilling Span based functions in .NET Standard - it's be much simpler just for .NET Core
…ed using' code formatter complaints
Recuces memory allocations, and improves performance
…perty accessors don't need to look it up themselves.
… and an Encoding as separate values
Some musings based on comments in #409 and #411
Just a couple of places for testing, could be done elsewhere -
Most of the complexity is pollyfilling Span based functions in .NET Standard and that can be tuned if this goes anywhere, and the same changes could be done elsewhere as well.
So, this is a combination of correctness (using ReadExactly instead of ReadBytes which might return less data than asked for), and performance (removes some allocations, removes some duplicate work)