Avoid unnecessary byte[] allocations in HttpContent#2493
Conversation
There was a problem hiding this comment.
The only place you're constructing these, you're passing in SomeEncoding and SomeEncoding.GetPreamble(). You could simplify those call sites by just making the constructor be:
public EncodingPreamblePair(Encoding encoding)
{
Encoding = encoding;
Preamble = encoding.GetPreamble();
}or you could add a constructor that does that:
public EncodingPreamblePair(Encoding encoding) : this(encoding, encoding.GetPreamble())
{
}
public EncodingPreamblePair(Encoding encoding, byte[] preamble)
{
Encoding = encoding;
Preamble = preamble;
}There was a problem hiding this comment.
Ah, yes, thanks! (I didn't notice the possible simplification because I was originally using KeyValuePair<K,V>). Fixed.
There was a problem hiding this comment.
I'm not convinced this should be a struct. Since we're only ever constructing a handful of these things, but we're iterating through the array that stores them potentially a lot, seems like it could be better to keep the size of the data being enumerated at one instead of two references.
There was a problem hiding this comment.
What'd you have in mind? Two arrays (one for the encodings and one for the byte[] preambles, that share the same indexes)? Or just one array of byte[] preambles with known indexes to get the associated encoding?
There was a problem hiding this comment.
struct is fine in this case, it's actually better than class as it avoids an additional indirection. struct would have been an issue if it was large and something like List<> would have been used instead of an array.
There was a problem hiding this comment.
I'll play around with a couple alternative approaches and measure.
There was a problem hiding this comment.
@mikedn, you're probably right, but I'm not sure what's wrong with my microbenchmarking then. Here's my example:
using System;
using System.Diagnostics;
using System.Linq;
static class Test
{
sealed class WrapperClass
{
public object Object1;
public object Object2;
}
struct WrapperStruct
{
public object Object1;
public object Object2;
}
static void Main()
{
var classArr = Enumerable.Range(0, 10).Select(_ => new WrapperClass { Object1 = new object(), Object2 = new object() }).ToArray();
var structArr = Enumerable.Range(0, 10).Select(_ => new WrapperStruct { Object1 = new object(), Object2 = new object() }).ToArray();
var sw = new Stopwatch();
const int Iters = 100000000;
while (true)
{
sw.Restart();
for (int i = 0; i < Iters; i++)
{
foreach (var item in structArr)
{
object obj1 = item.Object1;
object obj2 = item.Object2;
}
}
Console.WriteLine("Struct : " + sw.ElapsedMilliseconds);
sw.Restart();
for (int i = 0; i < Iters; i++)
{
foreach (var item in classArr)
{
object obj1 = item.Object1;
object obj2 = item.Object2;
}
}
Console.WriteLine("Class : " + sw.ElapsedMilliseconds);
Console.WriteLine();
}
}
}With VS2015 RTM, on my machine the 32-bit JIT results in the class being 3x the speed of the struct version, and the 64-bit JIT results in it being 1.25x faster. Do you see different results?
There was a problem hiding this comment.
JIT fail. In the class case it partially eliminates some the code inside the loop, in the struct case it insists on copying the struct fields to obj1 and obj2. If you change both loop bodies to
if (item.Object1 == item.Object2)
Console.WriteLine("eq");you'll get similar results for both struct and class. The class case is still a tiny bit faster due to other JIT issues but the difference is small enough that avoiding some heap allocations seems preferable.
That's with RyuJIT, I haven't tested with the x86 JIT. Is this code ever supposed to run on x86? As is now CoreCLR doesn't quite support x86.
There was a problem hiding this comment.
Is this code ever supposed to run on x86? As is now CoreCLR doesn't quite support x86.
CoreCLR is meant to support 32-bit, at least on Windows, and in fact we currently run most of our tests on Windows on 32-bit (though hopefully they'll soon run 64-bit by default). Plus many of these libraries are shared with .NET Native.
There was a problem hiding this comment.
Well, I checked the x86 results and it's funny, both struct and class generate good code yet the struct case is slower (1000ms vs 800ms). That's until you invert the order of the tests and then the struct case is slightly faster (730ms vs 770ms). Oh well, I guess we'll never get a realistic result by using such benchmarks. Anyone offers to flip a coin? 😄
There was a problem hiding this comment.
I flipped one 😄 The coin suggested sticking with struct.
|
Updated with an alternative approach that I landed on after trying a few alternatives. It's a little more code, but minimal allocations, and a lot faster than my initial commit:
PTAL (Note: I intend to squash all commits before merging). |
There was a problem hiding this comment.
With the way this has been restructured, is this else if necessary? I'm wondering if you can just move the body that sets bomLength up into the try/catch earlier, e.g.
try
{
encoding = Encoding.GetEncoding(innerThis.Headers.ContentType.CharSet);
bomLength = GetPreambleLength(data, dataLength, encoding);
}To me that makes all of this logic more clear:
- If there was a header for encoding, we try to get the encoding and its associated preamble length.
- If that wasn't successful, either because there wasn't a header or because we didn't recognize the encoding, we try to detect it from the data and get its associated BOM length
- Finally, if that wasn't successful, we use a default encoding and BOM length.
There was a problem hiding this comment.
(I'd probably also add a comment where bomLength is set to 0, highlighting that DefaultStringEncoding is UTF8, but we already checked to see if it had a UTF8 BOM, so the bomLength is 0.)
There was a problem hiding this comment.
Fixed. The logic is much clearer now, thanks for the feedback.
With the latest changes, the detection/fallback part looks like the following:
// If no content encoding is listed in the ContentType HTTP header, or no Content-Type header present,
// then check for a BOM in the data to figure out the encoding.
if (encoding == null)
{
TryDetectEncoding(data, dataLength, ref encoding, ref bomLength);
}
// Use the default encoding if we couldn't detect one.
if (encoding == null)
{
encoding = DefaultStringEncoding;
// DefaultStringEncoding is UTF8, but we already checked to see if it had a UTF8 BOM,
// so the bomLength is 0.
bomLength = 0;
}I'm mulling over changing TryDetectEncoding to actually return true/false, in which case the above could be:
// If no content encoding is listed in the ContentType HTTP header, or no Content-Type header present,
// then check for a BOM in the data to figure out the encoding.
if (encoding == null)
{
if (!TryDetectEncoding(data, dataLength, ref encoding, ref bomLength))
{
// Use the default encoding if we couldn't detect one.
encoding = DefaultStringEncoding;
// DefaultStringEncoding is UTF8, but we already checked to see if it had a UTF8 BOM,
// so the bomLength is 0.
bomLength = 0;
}
}Do you have a preference?
There was a problem hiding this comment.
Or, move the DefaultStringEncoding fallback directly inside TryDetectEncoding and keep it void (in which case I'd probably rename it just DetectEncoding).
// If no content encoding is listed in the ContentType HTTP header, or no Content-Type header present,
// then check for a BOM in the data to figure out the encoding with fallback to DefaultStringEncoding.
if (encoding == null)
{
DetectEncoding(data, dataLength, out encoding, out bomLength);
}There was a problem hiding this comment.
Do you have a preference?
My preference would be the Try version, as it makes it more clear to me at least what policy is being applied. If you did switch to the DetectEncoding version, my preference would be for it to return the encoding rather than using a ref, and to use out rather ref for the bomLength. Just my personal preference.
|
LGTM |
|
I have an even faster version of the encoding detection (~2-2.6x faster). I'll update the PR tomorrow with the improvements (sorry for the additional churn). |
|
I added a new commit with the improvements. |
|
@stephentoub Can you please review the latest commits to this? It changed since your previous LGTM. I'm preparing final testing of this. Thx. |
|
LGTM. My only question is, due to the hardcoding of various constants here, should we add some more Debug.Asserts, potentially in a cctor that's only under an |
|
@justinvp Can you comment on this? Thx. |
|
Alright, I added asserts for the encoding constants. PR updated and squashed into a single commit. |
|
Looks like the CI build is failing for unrelated reasons: |
HttpContent.ReadAsStringAsync() has a bunch of calls to Encoding.GetPreamble() as part of its encoding detection. GetPreamble() creates a new byte[] each time it is called. These byte[] allocations can be avoided. Also, as part of this, cleaned up and improved the performance of the encoding detection.
|
@dotnet-bot test this please |
|
LGTM |
|
LGTM2 |
Avoid unnecessary byte[] allocations in HttpContent
Avoid unnecessary byte[] allocations in HttpContent Commit migrated from dotnet/corefx@16abb16
8 participants
HttpContent.ReadAsStringAsync()has a bunch of calls toEncoding.GetPreamble()as part of its encoding detection.GetPreamble()creates a newbyte[]each time it is called. Thesebyte[]allocations can be avoided.