Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Use Buffer.MemoryCopy when available - #1036

Closed
benaadams wants to merge 3 commits into
aspnet:devfrom
benaadams:block-copy
Closed

Use Buffer.MemoryCopy when available#1036
benaadams wants to merge 3 commits into
aspnet:devfrom
benaadams:block-copy

Conversation

@benaadams

@benaadams benaadams commented Aug 6, 2016

Copy link
Copy Markdown
Contributor

MemoryCopy has improved a lot (and is faster than BlockCopy) dotnet/coreclr#6627

And is undergoing further improvements dotnet/coreclr#6638

@nietras

nietras commented Aug 6, 2016

Copy link
Copy Markdown

Have you checked the benchmarks I did for .NET 4.6 RyuJIT 64-bit, not sure MemoryCopy is the best approach yet ;)
image

@benaadams

benaadams commented Aug 6, 2016

Copy link
Copy Markdown
Contributor Author

Well its currently using BlockCopy so MemoryCopy is currently better 😄

And it looks like it will improve

@benaadams
benaadams force-pushed the block-copy branch 3 times, most recently from fb5bd9c to 152af3d Compare August 7, 2016 02:37
@benaadams

benaadams commented Aug 7, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Updated #1002 with further test failure info

cesarblum
cesarblum reviewed Aug 8, 2016
View reviewed changes
Comment thread src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIterator.cs Outdated
return Consume(count, out actual);
}

Debug.Assert(count + offset <= array.Length);

@cesarblum cesarblum Aug 8, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add message.

@benaadams benaadams Aug 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe should change it to actual throw?

@benaadams benaadams Aug 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also get the test the right way round XD

@muratg muratg added this to the Backlog milestone Aug 17, 2016
@benaadams

benaadams commented Sep 15, 2016

Copy link
Copy Markdown
Contributor Author

I'd hold on this - movement in coreclr on BlockCopy dotnet/coreclr#7198

@cesarblum

cesarblum commented Oct 20, 2016

Copy link
Copy Markdown
Contributor

@benaadams Any updates on this? dotnet/coreclr#7198 was merged almost a month ago.

@benaadams

benaadams commented Oct 20, 2016

Copy link
Copy Markdown
Contributor Author

@CesarBS that change is in post 1.1.0 😞

Could take this now and add issue to remeasure post 1.1.0?

@halter73

halter73 commented Nov 22, 2016

Copy link
Copy Markdown
Member

@benaadams Since this missed the 1.1 train and dotnet/coreclr#7198 is merged, should we close this?

@benaadams

benaadams commented Nov 29, 2016

Copy link
Copy Markdown
Contributor Author

Yeah, think so - landscape has changed. Might be worth a revalidation at some point; but don't think its worth keeping this PR open

@benaadams benaadams closed this Nov 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

6 participants

@benaadams @nietras @cesarblum @halter73 @muratg @dnfclas