Skip to content

Constant Encoded StringValues #64 #68

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.

Sign up for GitHub

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

Closed
JunTaoLuo wants to merge 4 commits into from
Closed

Constant Encoded StringValues #64 #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c14eea0
Constant Encoded StringValues #64
JunTaoLuo Dec 11, 2015
fc8e5c7
PR
JunTaoLuo Dec 12, 2015
9c608c8
Change StringValues to class
JunTaoLuo Dec 16, 2015
4671bc8
WIP
JunTaoLuo Jan 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 46 additions & 5 deletions src/Microsoft.Extensions.Primitives/StringValues.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Text;
using System.Collections;
using System.Collections.Generic;
using Microsoft.Extensions.Internal;
Expand All @@ -11,26 +12,66 @@ namespace Microsoft.Extensions.Primitives
/// <summary>
/// Represents zero/null, one, or many strings in an efficient way.
/// </summary>
public struct StringValues : IList<string>, IReadOnlyList<string>, IEquatable<StringValues>, IEquatable<string>, IEquatable<string[]>
public class StringValues : IList<string>, IReadOnlyList<string>, IEquatable<StringValues>, IEquatable<string>, IEquatable<string[]>
Copy link
Copy Markdown
Member

@Tratcher Tratcher Dec 16, 2015

Choose a reason for hiding this comment

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

Switching to a class you loose the default struct constructor. Should we add one?

Copy link
Copy Markdown

@pranavkm pranavkm Dec 16, 2015

Choose a reason for hiding this comment

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

Wasn't this a struct to avoid allocations?

Copy link
Copy Markdown
Member

@halter73 halter73 Dec 16, 2015

Choose a reason for hiding this comment

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

It was meant to avoid heap allocations. Turns out that benchmarking shows it wasn't worth it due to the increased copying.

Copy link
Copy Markdown
Author

@JunTaoLuo JunTaoLuo Dec 16, 2015

Choose a reason for hiding this comment

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

From testing though we see that the class implementation was better in terms of performance. It is likely that we are copying this type around very frequently and therefore incurring too much cost copying the fields. I also don't see a point in having a default constructor for the class. There's not much point in creating an instance will all fields as null instead of just setting the instance to null.

Copy link
Copy Markdown

@pranavkm pranavkm Dec 16, 2015

Choose a reason for hiding this comment

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

👍 The default ctor sems unnecessary. There isn't a whole lot of use for it.

{
private static readonly string[] EmptyArray = new string[0];
public static readonly StringValues Empty = new StringValues(EmptyArray);

private readonly string _value;
private readonly string[] _values;
private readonly Encoding _encoding;
private readonly byte[] _bytes;

public StringValues(string value)
{
_value = value;
_values = null;
}

public StringValues(string[] values)
{
_value = null;
_values = values;
}

private StringValues(string value, Encoding encoding)
{
_value = value;
_encoding = encoding;
_bytes = _encoding.GetBytes(value);
}

/// <summary>
/// Creates a new instance of <see cref="StringValues"/> with one string and compute its encoded bytes with the specified Encoding.
/// </summary>
/// <param name="value">The string to be encoded.</param>
/// <param name="encoding">The <see cref="Encoding"/> to be use when computing pre-encoded bytes.</param>
/// <returns>A <see cref="StringValues"/> which contains the pre-encoded bytes.</returns>
public static StringValues CreatePreEncoded(string value, Encoding encoding)
{
Copy link
Copy Markdown
Member

@Tratcher Tratcher Dec 16, 2015

Choose a reason for hiding this comment

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

We should null check the args in this scenario, we actually use them.

Copy link
Copy Markdown
Member

@halter73 halter73 Dec 16, 2015

Choose a reason for hiding this comment

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

👍

if (value == null)
{
throw new ArgumentNullException(nameof(value));
}
if (encoding == null)
{
throw new ArgumentNullException(nameof(encoding));
}

return new StringValues(value, encoding);
}

/// <summary>
/// Try to retrieve the pre-encoded bytes of the <see cref="StringValues"/>. The return value indicates whether pre-encoded bytes exist.
/// </summary>
/// <param name="bytes">If successful, <paramref name="bytes"/> contains the pre-encoded bytes of the <see cref="StringValues"/>.</param>
/// <param name="encoding">If successful, <paramref name="encoding"/> contains the <see cref="Encoding"/> used to compute the pre-encoded bytes.</param>
/// <returns><c>true</c> if <see cref="StringValues"/> contains pre-encoded bytes, otherwise <c>false</c>.</returns>
public bool TryGetPreEncoded(out byte[] bytes, out Encoding encoding)
Copy link
Copy Markdown

@pranavkm pranavkm Dec 14, 2015

Choose a reason for hiding this comment

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

This method looks kinda gross.

Copy link
Copy Markdown
Member

@Tratcher Tratcher Dec 14, 2015

Choose a reason for hiding this comment

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

Gross by design. Any recommendations for alternative designs?

Copy link
Copy Markdown

@pranavkm pranavkm Dec 14, 2015

Choose a reason for hiding this comment

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

Couldn't we make EncodedBytes a public property and add a remark that says its only available for pre-encoded values? Two outs feels like bad design.

Copy link
Copy Markdown
Member

@Tratcher Tratcher Dec 14, 2015

Choose a reason for hiding this comment

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

It's better to keep them linked than in two separate properties. Otherwise you need to keep explaining the relationship between the two.

{
bytes = _bytes;
encoding = _encoding;
return _bytes != null;
}

public static implicit operator StringValues(string value)
{
return new StringValues(value);
Expand Down Expand Up @@ -206,7 +247,7 @@ void ICollection<string>.Clear()

public Enumerator GetEnumerator()
{
return new Enumerator(ref this);
return new Enumerator(this);
}

IEnumerator<string> IEnumerable<string>.GetEnumerator()
Expand Down Expand Up @@ -424,7 +465,7 @@ public struct Enumerator : IEnumerator<string>
private string _current;
private int _index;

public Enumerator(ref StringValues values)
public Enumerator(StringValues values)
{
_values = values._values;
_current = values._value;
Expand Down
73 changes: 67 additions & 6 deletions test/Microsoft.Extensions.Primitives.Tests/StringValuesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Xunit;

namespace Microsoft.Extensions.Primitives
Expand All @@ -17,7 +18,6 @@ public static TheoryData<StringValues> DefaultOrNullStringValues
{
return new TheoryData<StringValues>
{
new StringValues(),
new StringValues((string)null),
new StringValues((string[])null),
(string)null,
Expand Down Expand Up @@ -63,7 +63,6 @@ public static TheoryData<StringValues, string> FilledStringValuesWithExpectedStr
{
return new TheoryData<StringValues, string>
{
{ default(StringValues), (string)null },
{ StringValues.Empty, (string)null },
{ new StringValues(new string[] { }), (string)null },
{ new StringValues(string.Empty), string.Empty },
Expand All @@ -79,7 +78,6 @@ public static TheoryData<StringValues, object> FilledStringValuesWithExpectedObj
{
return new TheoryData<StringValues, object>
{
{ default(StringValues), (object)null },
{ StringValues.Empty, (object)null },
{ new StringValues(new string[] { }), (object)null },
{ new StringValues("abc"), (object)"abc" },
Expand All @@ -96,7 +94,6 @@ public static TheoryData<StringValues, string[]> FilledStringValuesWithExpected
{
return new TheoryData<StringValues, string[]>
{
{ default(StringValues), new string[0] },
{ StringValues.Empty, new string[0] },
{ new StringValues(string.Empty), new[] { string.Empty } },
{ new StringValues("abc"), new[] { "abc" } },
Expand Down Expand Up @@ -333,8 +330,6 @@ public void DefaultNullOrEmpty_Concat(StringValues stringValues)
string[] empty = new string[0];
Assert.Equal(empty, StringValues.Concat(stringValues, StringValues.Empty));
Assert.Equal(empty, StringValues.Concat(StringValues.Empty, stringValues));
Assert.Equal(empty, StringValues.Concat(stringValues, new StringValues()));
Assert.Equal(empty, StringValues.Concat(new StringValues(), stringValues));
}

[Theory]
Expand Down Expand Up @@ -455,5 +450,71 @@ public void Equals_StringArray(StringValues stringValues, string[] expected)
Assert.True(StringValues.Equals(stringValues, expected));
Assert.False(StringValues.Equals(stringValues, notEqual));
}

[Theory]
[MemberData(nameof(DefaultOrNullStringValues))]
[MemberData(nameof(EmptyStringValues))]
[MemberData(nameof(FilledStringValues))]
public void TryGetConstant_NullDefaultEncodingAndEncodedBytes(StringValues stringValues)
{
Encoding encoding;
byte[] bytes;

Assert.False(stringValues.TryGetPreEncoded(out bytes, out encoding));
Assert.Null(encoding);
Assert.Null(bytes);
}

[Theory]
[MemberData(nameof(DefaultOrNullStringValues))]
[MemberData(nameof(EmptyStringValues))]
public void CreateConstant_ThrowsForNullOrEmpty(StringValues stringValues)
{
Assert.Throws<ArgumentNullException>(() => (StringValues.CreatePreEncoded(stringValues, Encoding.ASCII)));
}

[Fact]
public void CreateConstant_EncodesEmptyString()
Copy link
Copy Markdown
Author

@JunTaoLuo JunTaoLuo Dec 11, 2015

Choose a reason for hiding this comment

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

Some of these tests looks non-useful.

{
var stringValueConstant = StringValues.CreatePreEncoded("", Encoding.ASCII);
byte[] bytes;
Encoding encoding;

stringValueConstant.TryGetPreEncoded(out bytes, out encoding);

Assert.NotNull(bytes);
Assert.Empty(bytes);
}

[Fact]
public void CreateConstant_EncodesUsingSpecifiedEncoding()
{
var testString = "foo bar";
var expectedBytes = Encoding.ASCII.GetBytes(testString);
byte[] outBytes;
Encoding outEncoding;

var constantStringValues = StringValues.CreatePreEncoded(testString, Encoding.ASCII);
constantStringValues.TryGetPreEncoded(out outBytes, out outEncoding);

Assert.Equal(expectedBytes, outBytes);
Assert.Equal(Encoding.ASCII, outEncoding);
}

[Fact]
public void TryGetConstant_ReturnsIdenticalBytesAndEncoding()
{
var encoding = Encoding.GetEncoding(20127); // US-ASCII
var constantStringValues = StringValues.CreatePreEncoded("foo bar", encoding);

byte[] bytes1, bytes2;
Encoding encoding1, encoding2;
constantStringValues.TryGetPreEncoded(out bytes1, out encoding1);
constantStringValues.TryGetPreEncoded(out bytes2, out encoding2);

Assert.Same(bytes1, bytes2);
Assert.Same(encoding, encoding1);
Assert.Same(encoding, encoding2);
}
}
}