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

MemoryPool2 Stack->Queue, Allocate returns newest #310

Closed
wants to merge 1 commit into from

Conversation

benaadams
Copy link
Contributor

Rather than having a hot end for add and removal with a ConcurrentStack;
add to end and remove from top with ConcurrentQueue to reduce contention.

Rather than a while loop on Allocate; return last memory block created
rather than returning it to the pool and checking if one can be removed.

Behaviour remains unchanged (choice of collection is independent of api behaviour)

Rather than having a hot end for add and removal with a ConcurrentStack;
add to end and remove from to with Queue to reduce contention.

Rather than a while loop on Allocate; return last memory block created
rather than returning it to the pool.
@benaadams benaadams changed the title Stack->Queue, Allocate returns newest MemoryPool2 Stack->Queue, Allocate returns newest Nov 1, 2015
benaadams added a commit to benaadams/KestrelHttpServer that referenced this pull request Nov 1, 2015
@halter73
Copy link
Member

halter73 commented Nov 3, 2015

I haven't tested this. But intuitively, wouldn't a stack be more likely to reduce cache misses?

I think we should test this change in isolation before merging it.

@benaadams
Copy link
Contributor Author

Am evaluating currently; need to go faster though. Will close the PRs for now until I have some numbers?

@benaadams benaadams closed this Nov 3, 2015
@rynowak
Copy link
Member

rynowak commented Nov 3, 2015

Agreed that we should test this change in isolation. When @DamianEdwards and I looked at contention data we didn't see significant amounts of contention on the stack.

@benaadams
Copy link
Contributor Author

The issue is contention hits spinlocks in user mode before moving to kernel locks; under high contention they burn a lot of cpu; don't think spin locks are measured; only when they transition to kernel are they picked up. Have hit 1M rps sustaned #309 but latency has increased which means it gets worse at lower connection levels; so closed the uncertain PRs till can get to the bottom of it.

@rynowak
Copy link
Member

rynowak commented Nov 3, 2015

Ok cool, if we've actually seen data that shows this is a hotspot then that's different.

@rynowak
Copy link
Member

rynowak commented Nov 3, 2015

The issue is contention hits spinlocks in user mode before moving to kernel locks; under high contention they burn a lot of cpu; don't think spin locks are measured; only when they transition to kernel are they picked up

For stuff in System.Collections.Concurrent - they do ETW tracing for these events which is what we were looking at. Also all of these are lock-free, I don't believe you ever get kernal mode sync.

@benaadams
Copy link
Contributor Author

Just meant all the changes holistically together; this being one of - but latency goes from 6ms to 16ms; so the changes are untenable; which is why I've closed the PRs. Can always add more bandwidth, more cpu, more servers; but can't change the speed of light, so increasing latency is an awful outcome; and latency is a tcp killer.

My hope is that I can up the throughput and bring the latency back down; which should speed up the server so that these changes will be meaningful enough to be significant (good or bad) - at the moment they are just fiddling at the edges :-/

@benaadams
Copy link
Contributor Author

Raised PR #330 just for the allocate

@benaadams benaadams deleted the memory-pool-improvements branch May 10, 2016 11:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants