Use ContentLength as primary data source #1313

benaadams · 2017-01-20T00:57:43Z

React to IHeaderDictionary ContentLength change aspnet/HttpAbstractions#757

benaadams · 2017-01-20T07:22:41Z

Run through a debug and it does correctly call via the interface to FrameHeaders.ContentLength { get; set;} (When using the HttpAbstraction changes)

Tratcher · 2017-01-20T18:34:25Z

Hows the perf?

Tratcher · 2017-01-20T18:38:03Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameHeaders.Generated.cs

+            {
+                if (ContentLength.HasValue)
+                {
+                    return ContentLength.ToString();


HeaderUtilities.FormatInt64.
Would it be worth storing the string?

Changed all occurrences.

Don't think its worth storing the string; is another item to keep in sync; check if if exists/generate, clear. Is there a (non-test) scenario where its read multiple times as a string; when the numeric is available?

Tratcher · 2017-01-20T18:47:43Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/MessageBody.cs

@@ -266,18 +268,15 @@ protected virtual void OnConsumedBytes(int count)
                return new ForChunkedEncoding(keepAlive, headers, context);
            }

-            var unparsedContentLength = headers.HeaderContentLength;
-            if (unparsedContentLength.Count > 0)
+            if (headers.ContentLength.HasValue)


is there still a 400 produced for an invalid content-length header?

Is now (was throwing wrong exception, had to split request/response); is thrown during header parsing rather than message body - have added test.

Not certain its logged in same way though... looking into it

benaadams · 2017-01-20T21:07:44Z

The api change is having the desired effect on the Content-Length

Prior

         Method |                 Type |          Mean |    StdDev |           RPS | Allocated |
--------------- |--------------------- |-------------- |---------- |-------------- |---------- |
 BenchmarkAsync | ContentLengthNumeric |    92.4213 ns | 0.3669 ns | 10,820,017.35 |      32 B |
 BenchmarkAsync |  ContentLengthString |    65.1715 ns | 0.6884 ns | 15,344,128.34 |       0 B |
 BenchmarkAsync |            Plaintext |   128.8769 ns | 1.2060 ns |  7,759,339.56 |       0 B |
 BenchmarkAsync |              Primary |   239.4012 ns | 2.5521 ns |  4,177,087.82 |       0 B |
 BenchmarkAsync |              Unknown |   568.3201 ns | 4.6791 ns |  1,759,571.90 |       0 B |

This PR

         Method |                 Type |          Mean |     StdDev |           RPS | Allocated |
--------------- |--------------------- |-------------- |----------- |-------------- |---------- |
 BenchmarkAsync | ContentLengthNumeric |    87.9329 ns |  0.3409 ns | 11,372,308.13 |      32 B |
 BenchmarkAsync |  ContentLengthString |    66.5197 ns |  1.1896 ns | 15,033,135.95 |       0 B |
 BenchmarkAsync |            Plaintext |   136.7069 ns |  1.6866 ns |  7,314,920.75 |       0 B |
 BenchmarkAsync |              Primary |   256.5135 ns |  2.8525 ns |  3,898,430.80 |       0 B |
 BenchmarkAsync |              Unknown |   591.6517 ns | 12.8782 ns |  1,690,183.69 |       0 B |

This PR with HttpAbstractions

         Method |                 Type |          Mean |     StdDev |           RPS |
--------------- |--------------------- |-------------- |----------- |-------------- |
 BenchmarkAsync | ContentLengthNumeric |    21.8180 ns |  0.2589 ns | 45,833,772.51 |
 BenchmarkAsync |  ContentLengthString |    66.1873 ns |  0.7702 ns | 15,108,635.21 |
 BenchmarkAsync |            Plaintext |   137.8112 ns |  0.8571 ns |  7,256,306.67 |
 BenchmarkAsync |              Primary |   256.3452 ns |  2.2609 ns |  3,900,989.87 |
 BenchmarkAsync |              Unknown |   592.9667 ns |  7.4288 ns |  1,686,435.44 |

However, am getting a perf decrease overall when more headers are added which am looking into

benaadams · 2017-01-20T21:29:39Z

The test is just for adding headers not for response output 😒

halter73 · 2017-01-20T20:00:01Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs

                {
                    if (appCompleted && StatusCode != StatusCodes.Status101SwitchingProtocols)
                    {
                        // Since the app has completed and we are only now generating
                        // the headers we can safely set the Content-Length to 0.
-                        responseHeaders.SetRawContentLength("0", _bytesContentLengthZero);
+                        responseHeaders.ContentLength = 0;


Wouldn't this be slower than SetRawContentLength?

Is an extra 8 byte field in every header set to make the occasional 1 byte write of 0 slightly faster (as most responses will have some content) - can always special case 0 in the copyfrom..?

Your justification makes sense.

halter73 · 2017-01-20T23:10:30Z

test/Microsoft.AspNetCore.Server.KestrelTests/FrameResponseHeadersTests.cs

-        }
-
-        [Theory]
-        [MemberData(nameof(BadContentLengths))]
        public void ThrowsWhenAssigningHeaderContentLengthToNonNumericValue(string contentLength)


It doesn't look like BadContentLengths contains a negative value. I think we should include that. It would be nice if BadRequestIfContentLengthInvalid also included a test case for negative values.

I would also add a test specifically for setting a negative Content-Length using the new IHeaderDictionary property.

halter73 · 2017-01-20T23:19:49Z

tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/KnownHeaders.cs

+                            {{{(header.Identifier == "ContentLength" ? $@"
+                                if (ContentLength.HasValue)
+                                {{
+                                    ThrowInvalid{(className == "FrameResponseHeaders" ? "Response" : "Request")}ContentLengthException(AppendValue(HeaderUtilities.FormatInt64(ContentLength.Value), value).ToString());


Is this meant to guard against more than one Content-Length header? If so, an error message like "Invalid Content-Length: \"{value}\". Value must be a positive integral number." seems misleading.

Will add extra exception

Does throwing from header parsing get logged ok, or do I have to call logger?

Throwing a BadHttpRequestException should be sufficient, since it's supposed to be caught and logged in RequestProcessingAsync.

It looks like SetBadRequestState no longer logs the error and our tests didn't catch it though... 😢

Is sometimes https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs#L1506-L1510

The logging is broken when throwing a BadHttpRequestException without using RejectRequest() for cases when the exception can't easily be thrown from inside the Frame class as is the case here.

A situation very similar to the one you're running into now can be found elsewhere in this class.

This commit regressed the logging, but I can't find the PR that changed it. The commit seems to have been pushed sometime between #1163 and #1166.

@CesarBS Can you take a look? We can create an issue if it requires a big change.

This PR? #993

Good find. I wasn't looking through @mikeharder's PRs.

Throwing multiple content lengths instead

halter73 · 2017-01-20T23:22:55Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIterator.cs

+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static byte[] CreateNumericBytesScratch()
+        {
+            return (_numericBytesScratch = new byte[_maxPositiveLongByteLength]);


Don't love assignments inside expressions.

halter73 · 2017-01-20T23:35:45Z

tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/KnownHeaders.cs

-                    {Each(loop.Headers, header => $@"
-                        case {header.Index}:
-                            goto state{header.Index};
+                    {Each(loop.Headers.Where(header => header.Index >= 0), header => $@"


Using Index = -1 to distinguish the Content-Length header seems a little weird. Couldn't you just filter on Name?

halter73 · 2017-01-20T23:45:24Z

tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/KnownHeaders.cs

-            {(loop.ClassName == "FrameResponseHeaders" ? "_contentLength = null;" : "")}
-            if(FrameHeaders.BitCount(_bits) > 12)
+            ContentLength = null;
+            if(FrameHeaders.BitCount(_bits) > 11)


Wasn't this just a perf optimization for the case a lot of headers (specifically in _headers which now wouldn't include Content-Length) were set? Why would require less headers to be set to take this path now?

halter73 · 2017-01-20T23:53:08Z

tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/KnownHeaders.cs

-            }).Concat(corsRequestHeaders).Select((header, index) => new KnownHeader
+            })
+            .Concat(corsRequestHeaders)
+            .Where((header) => header != "Content-Length")


Would it be cleaner just to remove "Content-Length" from commonHeaders?

halter73 · 2017-01-21T00:00:03Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameHeaders.cs

        protected Dictionary<string, StringValues> Unknown => MaybeUnknown ?? (MaybeUnknown = new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase));

+        public long? ContentLength { get; set; }


We should have a custom setter, so we can throw immediately if this is set to a negative value or _isReadOnly is true.

With throw an InvalidOp for both Request and Response as will need to be manually set rather than input Request set (so server error 500)

ArgumentOutOfRangeException instead

benaadams · 2017-01-21T03:12:02Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIterator.cs

+            return (_numericBytesScratch = new byte[_maxPositiveLongByteLength]);
+        }
+
+        public void CopyFromNumeric(long value)


Changing to ulong as it doesn't handle negatives; either that or make internal

benaadams · 2017-01-21T04:43:30Z

Added bunch more tests; resolved feedback - other than GeneratedHeaders stuff, want to resolve the perf regression

Append() before 80 bytes, after 64 bytes zeroed AppendNonPrimaryHeaders() before 888 bytes, after 48 bytes zeroed

benaadams · 2017-01-22T17:39:35Z

Ok... went down this "lost performance" rabbit hole as was setting the Content-Length as string in some of the tests 🤒

Anyway, improves found along the way...

benaadams · 2017-01-22T18:18:45Z

Looking better

Pre

                 Type |          Mean |           RPS | Allocated |
--------------------- |-------------- |-------------- |---------- |
               Common | 1,059.8157 ns |    943,560.26 |      31 B |
 ContentLengthNumeric |    84.4719 ns | 11,838,254.08 |      31 B |
  ContentLengthString |    58.5621 ns | 17,075,894.64 |       0 B |
            Plaintext |   156.3607 ns |  6,395,468.47 |      31 B |
              Unknown |   697.8674 ns |  1,432,937.05 |      31 B |

Post

                 Type |          Mean |           RPS | Allocated |
--------------------- |-------------- |-------------- |---------- |
               Common | 1,083.2790 ns |    923,123.25 |      31 B |
 ContentLengthNumeric |    97.4643 ns | 10,260,171.78 |      31 B |
  ContentLengthString |    76.1497 ns | 13,132,024.10 |       0 B |
            Plaintext |   146.6288 ns |  6,819,943.26 |      31 B |
              Unknown |   734.3215 ns |  1,361,801.27 |      31 B |

Post + HttpAbstractions

                 Type |          Mean |           RPS |
--------------------- | ------------- |-------------- |
               Common |   988.2274 ns |  1,011,912.81 |
 ContentLengthNumeric |    41.3301 ns | 24,195,469.03 |
  ContentLengthString |    76.1173 ns | 13,137,625.51 |
            Plaintext |    82.6533 ns | 12,098,724.17 |
              Unknown |   567.9350 ns |  1,760,764.73 |

Still need to clean-up KnownHeaders

benaadams · 2017-01-22T22:46:56Z

Not happy with Output perf; working on it 😢

benaadams · 2017-01-23T04:55:09Z

Fixed the Header output

Before

        Method |                 Type |          Mean |     StdDev |           RPS |
-------------- |--------------------- |-------------- |----------- |-------------- |
 OutputHeaders | ContentLengthNumeric |    80.0507 ns |  1.9779 ns | 12,492,079.57 |
 OutputHeaders |            Plaintext |   146.3276 ns |  1.6375 ns |  6,833,978.82 |
 OutputHeaders |               Common |   831.6703 ns | 12.6436 ns |  1,202,399.61 |
 OutputHeaders |              Unknown | 1,213.7093 ns | 10.6639 ns |    823,920.50 |

After

        Method |                 Type |          Mean |     StdDev |           RPS |
-------------- |--------------------- |-------------- |----------- |-------------- |
 OutputHeaders | ContentLengthNumeric |    66.9508 ns |  0.4847 ns | 14,936,348.83 |
 OutputHeaders |            Plaintext |   135.2518 ns |  2.1630 ns |  7,393,616.26 |
 OutputHeaders |               Common |   817.0060 ns |  9.4297 ns |  1,223,981.22 |
 OutputHeaders |              Unknown | 1,234.0749 ns | 11.5047 ns |    810,323.57 |

cesarblum · 2017-01-23T17:46:49Z

src/Microsoft.AspNetCore.Server.Kestrel/BadHttpRequestException.cs

@@ -43,6 +43,9 @@ internal static BadHttpRequestException GetException(RequestRejectionReason reas
                case RequestRejectionReason.MalformedRequestInvalidHeaders:
                    ex = new BadHttpRequestException("Malformed request: invalid headers.", StatusCodes.Status400BadRequest);
                    break;
+                case RequestRejectionReason.MultipleContentLengths:
+                    ex = new BadHttpRequestException("Multiple content length headers.", StatusCodes.Status400BadRequest);


nit: use header name Content-Length

cesarblum · 2017-01-23T17:53:37Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/MessageBody.cs

+            {
+                if (count > 0)
+                {
+                    throw new InvalidDataException("Consuming non-existant data");


non-existent

cesarblum · 2017-01-23T17:57:18Z

test/Microsoft.AspNetCore.Server.KestrelTests/BadHttpRequestTests.cs

@@ -202,6 +202,19 @@ public class BadHttpRequestTests
            }
        }

+        [Fact]
+        public async Task BadRequestIfContentLengthInvalid()


Test with negative value too.

cesarblum · 2017-01-23T17:59:14Z

test/Microsoft.AspNetCore.Server.KestrelTests/FrameResponseHeadersTests.cs

@@ -151,6 +153,86 @@ public void ThrowsWhenClearingHeadersAfterReadOnlyIsSet()
            Assert.Throws<InvalidOperationException>(() => dictionary.Clear());
        }

+        [Fact]
+        public void CorrectContentLengthsOutput()


This should be in MemoryPoolIteratorTests.

Tratcher · 2017-01-23T23:13:30Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameHeaders.cs

        protected Dictionary<string, StringValues> Unknown => MaybeUnknown ?? (MaybeUnknown = new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase));

+        public long? ContentLength {


nit: style, bracket on new line

benaadams · 2017-01-24T02:09:16Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameHeaders.cs

@@ -412,11 +423,26 @@ public static unsafe TransferCoding GetFinalTransferCoding(StringValues transfer
            return transferEncodingOptions;
        }

-        private static void ThrowInvalidContentLengthException(string value)
+        protected static void ThrowInvalidResponseContentLengthException(long value)


These exceptions can move to respective classes (Request/Response)

benaadams · 2017-01-24T02:09:26Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameHeaders.cs

+            throw new ArgumentOutOfRangeException($"Invalid Content-Length: \"{value}\". Value must be a positive integral number.");
+        }
+
+        protected static void ThrowInvalidResponseContentLengthException(string value)


benaadams · 2017-01-24T02:09:35Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameHeaders.cs

        {
            throw new InvalidOperationException($"Invalid Content-Length: \"{value}\". Value must be a positive integral number.");
        }

+        protected static void ThrowInvalidRequestContentLengthException(string value)


halter73 · 2017-01-24T19:33:16Z

tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/KnownHeaders.cs

@@ -507,35 +533,46 @@ protected void CopyToFast(ref MemoryPoolIterator output)
                        }}
                    }}

+                    if({header.TestNotTempBit()})


Do we know this is actually faster than doing the tempBits &= ~{1L << header.Index}L; before the if condition and then comparing tempBits to zero. If so, why are you doing it the old way just for the Content-Length header?

Yes is faster, measured before and after, significant but not vast; missed content-length accidentally because its now a snowflake 😢

Looks like I completely missed a commit 😱

halter73 · 2017-01-24T19:47:22Z

tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/KnownHeaders.cs

+                EnhancedSetter = enhancedHeaders.Contains("Content-Length"),
+                PrimaryHeader = responsePrimaryHeaders.Contains("Content-Length")
+            }})
+            .ToArray();


Nit: We should have done this before, but can we assert that responseHeaders and requestHeaders have no more than 64 items and/or the max Index is 62.

63 for reponseHeaders as it steals one for Content-Length in CopyTo(ref MemoryPoolIterator output)

halter73 · 2017-01-24T19:49:34Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIterator.cs

+            {
+                if (bytesLeftInBlock < 1)
+                {
+                    goto overflow;


Is this that much better than just calling CopyFromNumericOverflow(value); and returning? I prefer avoiding goto if we can.

Not in a way I could probably sensibility justify... It generates the code I'd like the Jit to generate 😆

halter73 · 2017-01-24T20:12:34Z

pending last few comments

benaadams · 2017-01-24T20:57:02Z

Didn't pickup all my local changes in the final commit; fixed; also addressed feedback

benaadams · 2017-01-24T21:37:38Z

Most of the changes were in; though hadn't picked up the KnownHeaders file; though picked up the Generated one.

cesarblum · 2017-01-24T21:38:12Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameRequestHeaders.cs

+            throw BadHttpRequestException.GetException(RequestRejectionReason.InvalidContentLength, value);
+        }
+
+        private static void ThrowMultipleContentLengths()


nit: rename to ThrowMultipleContentLengthsException

dnfclas added the cla-already-signed label Jan 20, 2017

benaadams force-pushed the contentlength branch from 434afc9 to cdcc2c6 Compare January 20, 2017 08:15

Tratcher requested a review from cesarblum January 20, 2017 18:35

Tratcher reviewed Jan 20, 2017

View reviewed changes

Tratcher requested a review from halter73 January 20, 2017 18:53

Tratcher self-assigned this Jan 20, 2017

halter73 suggested changes Jan 21, 2017

View reviewed changes

benaadams commented Jan 21, 2017

View reviewed changes

benaadams added 6 commits January 21, 2017 05:04

Use ContentLength as primary data source

2c5dd5f

Zero contentLength msgbody doesn't need state

bc2cb10

Remove HasContentLength prop

e2615b2

Throw correct exception, FormatInt64, test

6a0c922

Add extra contentlength tests

4718d29

Throw multiple content lengths

421d1a1

benaadams force-pushed the contentlength branch from 56cdc67 to 421d1a1 Compare January 21, 2017 17:11

benaadams added 4 commits January 21, 2017 19:47

Cleanup headers

85a6655

Headers Cleanup

55515dd

Cut Append/AppendNonPrimaryHeaders zeroed stack

e330a9c

Append() before 80 bytes, after 64 bytes zeroed AppendNonPrimaryHeaders() before 888 bytes, after 48 bytes zeroed

Stop benchmarks crashing

99c9012

benaadams mentioned this pull request Jan 22, 2017

FrameRequestHeaders:AppendNonPrimaryHeaders uses (and zeros) 888 bytes of stack #1317

Closed

Fast ulong output

04229be

benaadams force-pushed the contentlength branch from 5847bfc to 04229be Compare January 23, 2017 04:53

benaadams added 3 commits January 23, 2017 06:10

Cleanup KnownHeaders

4b57d2c

Test before set

1a3f4bf

Clear test before set, use local var

6333f36

cesarblum suggested changes Jan 23, 2017

View reviewed changes

Feedback

a1bfe6b

benaadams mentioned this pull request Jan 23, 2017

[NoMerge] For davidfowl #1320

Closed

Tratcher reviewed Jan 23, 2017

View reviewed changes

benaadams commented Jan 24, 2017

View reviewed changes

Feedback

dc8004e

benaadams force-pushed the contentlength branch from 0f3b34e to dc8004e Compare January 24, 2017 02:21

halter73 reviewed Jan 24, 2017

View reviewed changes

cesarblum approved these changes Jan 24, 2017

View reviewed changes

Feedback

92b6bfa

cesarblum reviewed Jan 24, 2017

View reviewed changes

ThrowMultipleContentLengthsException

08ad325

Tratcher merged commit ecca980 into aspnet:dev Jan 24, 2017

benaadams deleted the contentlength branch January 24, 2017 23:07

benaadams mentioned this pull request May 23, 2017

Bring over a few changes from the TechEmpower repo aspnet/Benchmarks#266

Closed

		protected Dictionary<string, StringValues> Unknown => MaybeUnknown ?? (MaybeUnknown = new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase));

		public long? ContentLength { get; set; }

		protected Dictionary<string, StringValues> Unknown => MaybeUnknown ?? (MaybeUnknown = new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase));

		public long? ContentLength {

Use ContentLength as primary data source #1313

Use ContentLength as primary data source #1313

Conversation

benaadams commented Jan 20, 2017

benaadams commented Jan 20, 2017 • edited Loading

Tratcher commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Jan 20, 2017

benaadams commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Jan 21, 2017

benaadams commented Jan 22, 2017

benaadams commented Jan 22, 2017

benaadams commented Jan 22, 2017

benaadams commented Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 commented Jan 24, 2017

benaadams commented Jan 24, 2017 • edited Loading

benaadams commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Jan 20, 2017 •

edited

Loading

benaadams commented Jan 23, 2017 •

edited

Loading

benaadams commented Jan 24, 2017 •

edited

Loading