Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Performance regression after adding segment type switch #2046

Closed
pakrym opened this issue Jan 11, 2018 · 12 comments
Closed

Performance regression after adding segment type switch #2046

pakrym opened this issue Jan 11, 2018 · 12 comments

Comments

@pakrym
Copy link
Contributor

pakrym commented Jan 11, 2018

We've seen 10% regression in kestrel and the source seems to be that multiple ReadOnlyBuffer methods became less efficient after the change namely:

  1. Seek (because of CheckBounds)
  2. TryGet
  3. Reader.MoveNext - I suspect mostly because of TryGet
@pakrym
Copy link
Contributor Author

pakrym commented Jan 11, 2018

@benaadams
Copy link
Member

image

@benaadams
Copy link
Member

benaadams commented Jan 12, 2018

Aside, this might be helpful https://github.com/dotnet/coreclr/issues/15755

ref struct BufferReader<TSequence> doesn't need any write barriers in it

@KrzysztofCwalina
Copy link
Member

What was the TryGet change?

@pakrym
Copy link
Contributor Author

pakrym commented Jan 12, 2018

Adding 3 possible types of segments (Array, OwnedMemory and IBufferList)

@pakrym
Copy link
Contributor Author

pakrym commented Jan 12, 2018

@benaadams yeah, it generates pretty bad code around structs, and also bunch of calls to coreclr (IsArray, IsInstanceOfClass)

@benaadams
Copy link
Member

What was the TryGet change?

Pattern matching the different types?

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Jan 13, 2018

I talked with @pakrym and he will try to use the high order bit in one of the Position's ints to identify the IBufferList case. Maybe we can even use the other bit to identify T[]

@benaadams
Copy link
Member

benaadams commented Jan 13, 2018

Lose 2 bits; is still 1GB; if you need more you aren't segmenting correctly :)
Would also allow a 4th type at some point... (string for a StringBuilder type scenario?)

@halter73
Copy link
Member

@benaadams Those of us with email notifications can see your ninja edits 😆 .

@ahsonkhan
Copy link
Member

@pakrym, has this issue been resolved? Can it be closed?

@pakrym
Copy link
Contributor Author

pakrym commented Mar 12, 2018

Yes. And we shouldn't track it here anymore.

@pakrym pakrym closed this as completed Mar 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants