Feature/#258 #271

crfloyd · 2018-04-24T18:23:42Z

Closes #258

FEATURE

write tests that address the requirements outlined in the issue
fulfill the feature requirements
bump package version

… string as Span<char> given a char delimeter. Language version updated to 7.2 due to need for ref Struct to utilize Span<T> as a field.

…estMiddleware, JsonApiContext, and QueryParser to utilize new SpanSplitter library to avoid unnecessary intermediate string creation.

…'s logic. ContentNegotion tests pass.

crfloyd · 2018-04-24T18:48:12Z

This feature requires language version 7.2 for use of ref Struct to allow for usage of Span as a field. Appveyor build currently failing due to this.

jaredcnance · 2018-04-24T20:07:20Z

Thanks for doing this! I'll carve out some time soon to work on a review. Looks like the Travis build is blocked by travis-ci/travis-ci#8990

I'll do some research and see if there is a workaround, but I'm not terribly concerned as long as Travis is working on a fix. If we have to I can probably move the linux build over to CircleCI or something.

I'm also interested in writing some benchmarks to quantify the impact. If you wanted to try it out, you can take a look at some of the other benchmarks we have: https://github.com/json-api-dotnet/JsonApiDotNetCore/tree/master/benchmarks ...

Otherwise, I'll probably just add on to your PR when I have some time.

crfloyd · 2018-04-24T20:27:53Z

Sure, I can do some benchmarks. Ill work on that next. Thanks.

…ming to be more accurate

…pdates usages and unit tests to use extension method.

jaredcnance · 2018-04-28T13:44:04Z

src/JsonApiDotNetCore/Builders/LinkBuilder.cs

            }
-
-            return nSpace;
+            return sb.ToString();


Interestingly, as written this performs almost identically to the Split version. I've found one optimization that cuts this in half by removing the StringBuilder. I also think we can eliminate most of the allocations within SpanSplitter since I don't think it needs to be stateful. We really just need to read over a string and find the place where /{entityName} exists (followed by / or EOL), then we Slice everything before that. So, we shouldn't need to allocate anything until the final .Slice(/*...*/).ToString(). I'll push my changes sometime this weekend.

These are the results I get when running benchmarks (will submit code soon):

BenchmarkDotNet=v0.10.10, OS=Mac OS X 10.12 Processor=Intel Core i5-5257U CPU 2.70GHz (Broadwell), ProcessorCount=4 .NET Core SDK=2.1.4 [Host] : .NET Core 2.0.5 (Framework 4.6.0.0), 64bit RyuJIT Job-LPGSMZ : .NET Core 2.0.5 (Framework 4.6.0.0), 64bit RyuJIT LaunchCount=3 TargetCount=20 WarmupCount=10

Method Mean Error StdDev Median Gen 0 Allocated

UsingSplit (original) 1,257.5 ns 36.05 ns 76.83 ns 1,229.1 ns 0.9251 1456 B

UsingSpanWithStringBuilder (current PR) 1,770.0 ns 68.65 ns 144.81 ns 1,767.0 ns 0.9460 1488 B

UsingSpanWithoutStringBuilder 871.3 ns 17.22 ns 36.69 ns 858.9 ns 0.4368 688 B

UsingSpanWithoutSpanSplitter 317.2 ns 25.54 ns 56.06 ns 300.5 ns 0.0863 136 B

jaredcnance · 2018-05-02T02:27:23Z

src/JsonApiDotNetCore/Middleware/RequestMiddleware.cs

+            const char delimeter = ';';
+            var subSpans = mediaType.SpanSplit(delimeter);
+            if (subSpans.Count == 0) return false;
+            return subSpans.Count == 2 && subSpans[0].ToString() == Constants.ContentType;


So, it appears that the simple overhead of the SpanSplitter type definition and its members has enough cost that we don't actually improve the performance. In general, it appears we can get much better performance by not creating a new abstraction. While I feel like your approach was elegant, it unfortunately did not have the desired result.

In the coming commit, I was able to get this down to 0 allocations.

BenchmarkDotNet=v0.10.10, OS=Mac OS X 10.12 Processor=Intel Core i5-5257U CPU 2.70GHz (Broadwell), ProcessorCount=4 .NET Core SDK=2.1.4 [Host] : .NET Core 2.0.5 (Framework 4.6.0.0), 64bit RyuJIT DefaultJob : .NET Core 2.0.5 (Framework 4.6.0.0), 64bit RyuJIT

Method Mean Error StdDev Gen 0 Allocated

UsingSplit 157.11 ns 2.9374 ns 2.6039 ns 0.2134 336 B

UsingSpan (Current PR) 364.75 ns 7.1680 ns 6.3542 ns 0.2389 376 B

Proposal (6568c37) 37.91 ns 0.7256 ns 0.6787 ns - 0 B

jaredcnance · 2018-05-02T03:25:17Z

src/JsonApiDotNetCore/Services/JsonApiContext.cs

-            IsRelationshipPath = path[path.Length - 2] == "relationships";
+
+            var pathSpans = requestPath.SpanSplit('/');
+            IsRelationshipPath = pathSpans[pathSpans.Count - 2].ToString() == "relationships";


BenchmarkDotNet=v0.10.10, OS=Mac OS X 10.12 Processor=Intel Core i5-5257U CPU 2.70GHz (Broadwell), ProcessorCount=4 .NET Core SDK=2.1.4 [Host] : .NET Core 2.0.5 (Framework 4.6.0.0), 64bit RyuJIT DefaultJob : .NET Core 2.0.5 (Framework 4.6.0.0), 64bit RyuJIT

Method Mean Error StdDev Gen 0 Allocated

Original 421.08 ns 19.3905 ns 54.0529 ns 0.4725 744 B

Current PR 697.65 ns 11.9282 ns 11.1576 ns 0.5999 944 B

Proposal (fbe1d1b) 52.23 ns 0.8052 ns 0.7532 ns - 0 B

jaredcnance · 2018-05-06T22:03:49Z

Thanks @crfloyd for getting this started. Sorry we couldn't take all your changes but your efforts are greatly appreciated! Let me know if you have any questions/concerns over the changes.

crfloyd · 2018-05-07T00:54:47Z

No problem. Happy to help.

Feature/#258

Corey Floyd added 3 commits April 24, 2018 13:09

json-api-dotnet#258 Adds new utility to provide substrings of a given…

b85bc14

… string as Span<char> given a char delimeter. Language version updated to 7.2 due to need for ref Struct to utilize Span<T> as a field.

json-api-dotnet#258 Updates LinkBuilder, RelatedAttrFilterQuery, Requ…

9936939

…estMiddleware, JsonApiContext, and QueryParser to utilize new SpanSplitter library to avoid unnecessary intermediate string creation.

json-api-dotnet#258 Fixes Error in ContainsMediaTypeParameters method…

a48fc4c

…'s logic. ContentNegotion tests pass.

crfloyd added 2 commits April 24, 2018 20:31

json-api-dotnet#258 Removes extra ToString() on the span and fixes na…

cfa785f

…ming to be more accurate

json-api-dotnet#258 Adds extension method for splitting with spans. U…

85bb9f4

…pdates usages and unit tests to use extension method.

jaredcnance reviewed Apr 28, 2018

View reviewed changes

jaredcnance force-pushed the feature/#258 branch from d67f22d to 85bb9f4 Compare April 29, 2018 00:19

jaredcnance added 4 commits April 28, 2018 20:01

refactor(LinkBuilder): reduce allocations

01735f0

Update .travis.yml

4803fd0

benchamrk: don't duplicate the final version's definition

2f0e481

add benchmarks to RequestMiddleware

6568c37

jaredcnance reviewed May 2, 2018

View reviewed changes

add benchmarks for IsRelationshipPath

fbe1d1b

jaredcnance reviewed May 2, 2018

View reviewed changes

clean up unused changes

369860c

jaredcnance merged commit cee11aa into json-api-dotnet:master May 6, 2018

jaredcnance added a commit that referenced this pull request Aug 12, 2018

Merge pull request #271 from crfloyd/feature/#258

e8a0607

Feature/#258

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Feature/#258 #271

Feature/#258 #271

Uh oh!

crfloyd commented Apr 24, 2018 •

edited by jaredcnance

Loading

Uh oh!

crfloyd commented Apr 24, 2018

Uh oh!

jaredcnance commented Apr 24, 2018 •

edited

Loading

Uh oh!

crfloyd commented Apr 24, 2018

Uh oh!

jaredcnance Apr 28, 2018 •

edited

Loading

Uh oh!

jaredcnance Apr 28, 2018

Uh oh!

jaredcnance May 2, 2018

Uh oh!

jaredcnance May 2, 2018

Uh oh!

jaredcnance commented May 6, 2018

Uh oh!

crfloyd commented May 7, 2018

Uh oh!

Uh oh!

Method	Mean	Error	StdDev	Median	Gen 0	Allocated
UsingSplit (original)	1,257.5 ns	36.05 ns	76.83 ns	1,229.1 ns	0.9251	1456 B
UsingSpanWithStringBuilder (current PR)	1,770.0 ns	68.65 ns	144.81 ns	1,767.0 ns	0.9460	1488 B
UsingSpanWithoutStringBuilder	871.3 ns	17.22 ns	36.69 ns	858.9 ns	0.4368	688 B
UsingSpanWithoutSpanSplitter	317.2 ns	25.54 ns	56.06 ns	300.5 ns	0.0863	136 B

Method	Mean	Error	StdDev	Gen 0	Allocated
UsingSplit	157.11 ns	2.9374 ns	2.6039 ns	0.2134	336 B
UsingSpan (Current PR)	364.75 ns	7.1680 ns	6.3542 ns	0.2389	376 B
Proposal (`6568c37`)	37.91 ns	0.7256 ns	0.6787 ns	-	0 B

Method	Mean	Error	StdDev	Gen 0	Allocated
Original	421.08 ns	19.3905 ns	54.0529 ns	0.4725	744 B
Current PR	697.65 ns	11.9282 ns	11.1576 ns	0.5999	944 B
Proposal (`fbe1d1b`)	52.23 ns	0.8052 ns	0.7532 ns	-	0 B

Uh oh!

Feature/#258 #271

Feature/#258 #271

Uh oh!

Conversation

crfloyd commented Apr 24, 2018 • edited by jaredcnance Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

FEATURE

Uh oh!

crfloyd commented Apr 24, 2018

Uh oh!

jaredcnance commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crfloyd commented Apr 24, 2018

Uh oh!

jaredcnance Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredcnance Apr 28, 2018

Choose a reason for hiding this comment

Uh oh!

jaredcnance May 2, 2018

Choose a reason for hiding this comment

Uh oh!

jaredcnance May 2, 2018

Choose a reason for hiding this comment

Uh oh!

jaredcnance commented May 6, 2018

Uh oh!

crfloyd commented May 7, 2018

Uh oh!

Uh oh!

crfloyd commented Apr 24, 2018 •

edited by jaredcnance

Loading

jaredcnance commented Apr 24, 2018 •

edited

Loading

jaredcnance Apr 28, 2018 •

edited

Loading