Change JumpTable to take ReadOnlySpan #22397
Conversation
ghost
added
the
| @@ -663,37 +663,6 @@ internal Candidate CreateCandidate(Endpoint endpoint, int score) | |||
| } | |||
| } | |||
|
|
|||
| private int[] GetGroupLengths(DfaNode node) | |||
| { | ||
| return _exitDestination; | ||
| } | ||
|
|
||
| var text = path.Substring(segment.Start, segment.Length); | ||
| var text = path.ToString(); |
There was a problem hiding this comment.
@GrabYourPitchforks @stephentoub @jkotas it would be great to allow lookup by ReadOnlySpan<char> without the added allocation (closest issue I saw was this dotnet/runtime#27229)
There was a problem hiding this comment.
Yes. The sticking point has been with a design regarding custom comparers:
dotnet/runtime#28368 (comment)
There was a problem hiding this comment.
One other sticking point is that there's no guarantee that the same algorithm is used to compute the hash code of a string vs. the hash code of a ReadOnlySpan<char>. In fact, Dictionary<string, ...> throws away the default comparer and uses a specialized string comparer for greater performance. It's definitely solvable, but doing this in a manner that won't regress performance of existing scenarios is a much more complex problem than it appears.
Edit: The reason this matters is that the hash code is needed to compute which bucket the key maps to. If we inadvertently compute the wrong hash code, we could end up in a situation where the dictionary truly does contain some string value but we say it doesn't contain the equivalent ROS<char> value.
There was a problem hiding this comment.
I get it but how do we make progress? Does somebody just have to ask again? Do we have proposals?
There was a problem hiding this comment.
My proposal is what's in my linked comment: throw from the method if the comparer being used is not one we implicitly understand and can handle internally appropriately. It's not perfect, it's not beautiful, but it addresses what I think is the 99% case: using one of the built-in comparers. The downside is there's a big cliff that manifests as either an exception or string allocation when the comparer isn't supported. If we went with exception, we could make it work in the future if we added another interface supporting spans we could query for.
There was a problem hiding this comment.
There are a few candidate proposals in dotnet/runtime#27229. (But Steve, I don't see your idea reflected there? Maybe I'm looking at the wrong piece?) The issue is currently marked as needs work and has no assignee. Somebody would need to champion it and drive it through API review.
Yea me too, I can't get it running.. |
|
OK the microbenchmarks are broken in master 😞 |
- Small change while trying to get a better handle on the part of the code base. Today we pass a string and 2 pointers and instead we can pass a ReadOnlySpan. - Updates tests and IL generator to handle the new signature.
davidfowl
force-pushed
the
davidfowl/span-routing
branch
from
ecb5dd1
to
6e50aa2
Compare
amcasey
added
area-networking
TODO: Run perf tests