Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change JumpTable to take ReadOnlySpan #22397

Closed
wants to merge 2 commits into from
Closed

Change JumpTable to take ReadOnlySpan #22397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81597c1
Change JumpTable to take ReadOnlySpan
davidfowl May 31, 2020
6e50aa2
PR feedback
davidfowl Jun 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/Http/Routing/perf/Matching/JumpTableMultipleEntryBenchmark.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -96,7 +96,7 @@ public int LinearSearch()
var destination = 0;
for (var i = 0; i < strings.Length; i++)
{
destination = _linearSearch.GetDestination(strings[i], segments[i]);
destination = _linearSearch.GetDestination(strings[i].AsSpan(segments[i].Start, segments[i].Length));
}

return destination;
Expand All @@ -111,7 +111,7 @@ public int Dictionary()
var destination = 0;
for (var i = 0; i < strings.Length; i++)
{
destination = _dictionary.GetDestination(strings[i], segments[i]);
destination = _dictionary.GetDestination(strings[i].AsSpan(segments[i].Start, segments[i].Length));
}

return destination;
Expand All @@ -126,7 +126,7 @@ public int Trie()
var destination = 0;
for (var i = 0; i < strings.Length; i++)
{
destination = _trie.GetDestination(strings[i], segments[i]);
destination = _trie.GetDestination(strings[i].AsSpan(segments[i].Start, segments[i].Length));
}

return destination;
Expand All @@ -141,7 +141,7 @@ public int VectorTrie()
var destination = 0;
for (var i = 0; i < strings.Length; i++)
{
destination = _vectorTrie.GetDestination(strings[i], segments[i]);
destination = _vectorTrie.GetDestination(strings[i].AsSpan(segments[i].Start, segments[i].Length));
}

return destination;
Expand Down
10 changes: 5 additions & 5 deletions src/Http/Routing/perf/Matching/JumpTableSingleEntryBenchmark.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -88,7 +88,7 @@ public int Default()
var destination = 0;
for (var i = 0; i < strings.Length; i++)
{
destination = _default.GetDestination(strings[i], segments[i]);
destination = _default.GetDestination(strings[i].AsSpan(segments[i].Start, segments[i].Length));
}

return destination;
Expand All @@ -103,7 +103,7 @@ public int Ascii()
var destination = 0;
for (var i = 0; i < strings.Length; i++)
{
destination = _ascii.GetDestination(strings[i], segments[i]);
destination = _ascii.GetDestination(strings[i].AsSpan(segments[i].Start, segments[i].Length));
}

return destination;
Expand All @@ -118,7 +118,7 @@ public int Trie()
var destination = 0;
for (var i = 0; i < strings.Length; i++)
{
destination = _trie.GetDestination(strings[i], segments[i]);
destination = _trie.GetDestination(strings[i].AsSpan(segments[i].Start, segments[i].Length));
}

return destination;
Expand All @@ -133,7 +133,7 @@ public int VectorTrie()
var destination = 0;
for (var i = 0; i < strings.Length; i++)
{
destination = _vectorTrie.GetDestination(strings[i], segments[i]);
destination = _vectorTrie.GetDestination(strings[i].AsSpan(segments[i].Start, segments[i].Length));
}

return destination;
Expand Down
5 changes: 3 additions & 2 deletions src/Http/Routing/perf/Matching/JumpTableZeroEntryBenchmark.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using BenchmarkDotNet.Attributes;

namespace Microsoft.AspNetCore.Routing.Matching
Expand Down Expand Up @@ -57,7 +58,7 @@ public int Implementation()
var destination = 0;
for (var i = 0; i < strings.Length; i++)
{
destination = _table.GetDestination(strings[i], segments[i]);
destination = _table.GetDestination(strings[i].AsSpan(segments[i].Start, segments[i].Length));
}

return destination;
Expand Down
3 changes: 2 additions & 1 deletion src/Http/Routing/src/Matching/DfaMatcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,8 @@ public sealed override Task MatchAsync(HttpContext httpContext)
var destination = 0;
for (var i = 0; i < segments.Length; i++)
{
destination = states[destination].PathTransitions.GetDestination(path, segments[i]);
var segment = segments[i];
destination = states[destination].PathTransitions.GetDestination(path.AsSpan(segment.Start, segment.Length));
}

// Process an arbitrary number of policy-based decisions
Expand Down
33 changes: 1 addition & 32 deletions src/Http/Routing/src/Matching/DfaMatcherBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ private static int GetPrecedenceDigitAtDepth(RouteEndpoint endpoint, int depth)
if (segment is null)
{
// Treat "no segment" as high priority. it won't effect the algorithm, but we need to define a sort-order.
return 0;
return 0;
}

return RoutePrecedence.ComputeInboundPrecedenceDigit(endpoint.RoutePattern, segment);
Expand Down Expand Up @@ -663,37 +663,6 @@ internal Candidate CreateCandidate(Endpoint endpoint, int score)
}
}

private int[] GetGroupLengths(DfaNode node)
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused

{
var nodeMatches = node.Matches;
if (nodeMatches == null || nodeMatches.Count == 0)
{
return Array.Empty<int>();
}

var groups = new List<int>();

var length = 1;
var exemplar = nodeMatches[0];

for (var i = 1; i < nodeMatches.Count; i++)
{
if (!_comparer.Equals(exemplar, nodeMatches[i]))
{
groups.Add(length);
length = 0;

exemplar = nodeMatches[i];
}

length++;
}

groups.Add(length);

return groups.ToArray();
}

private static bool HasAdditionalRequiredSegments(RouteEndpoint endpoint, int depth)
{
for (var i = depth; i < endpoint.RoutePattern.PathSegments.Count; i++)
Expand Down
8 changes: 4 additions & 4 deletions src/Http/Routing/src/Matching/DictionaryJumpTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -29,14 +29,14 @@ public DictionaryJumpTable(
}
}

public override int GetDestination(string path, PathSegment segment)
public override int GetDestination(ReadOnlySpan<char> path)
{
if (segment.Length == 0)
if (path.Length == 0)
{
return _exitDestination;
}

var text = path.Substring(segment.Start, segment.Length);
var text = path.ToString();
Copy link
Member Author

Choose a reason for hiding this comment

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

@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)

Copy link
Member

Choose a reason for hiding this comment

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

Yes. The sticking point has been with a design regarding custom comparers:
dotnet/runtime#28368 (comment)

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I get it but how do we make progress? Does somebody just have to ask again? Do we have proposals?

Copy link
Member

@stephentoub stephentoub Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if (_dictionary.TryGetValue(text, out var destination))
{
return destination;
Expand Down
44 changes: 22 additions & 22 deletions src/Http/Routing/src/Matching/ILEmitTrieFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand All @@ -20,7 +20,7 @@ internal static class ILEmitTrieFactory
// Creates a Func of (string path, int start, int length) => destination
// Not using PathSegment here because we don't want to mess with visibility checks and
// generating IL without it is easier.
public static Func<string, int, int, int> Create(
public static JumpTableDelegate Create(
int defaultDestination,
int exitDestination,
(string text, int destination)[] entries,
Expand All @@ -29,15 +29,15 @@ public static Func<string, int, int, int> Create(
var method = new DynamicMethod(
"GetDestination",
typeof(int),
new[] { typeof(string), typeof(int), typeof(int), });
new[] { typeof(ReadOnlySpan<char>) });

GenerateMethodBody(method.GetILGenerator(), defaultDestination, exitDestination, entries, vectorize);

#if IL_EMIT_SAVE_ASSEMBLY
SaveAssembly(method.GetILGenerator(), defaultDestination, exitDestination, entries, vectorize);
#endif

return (Func<string, int, int, int>)method.CreateDelegate(typeof(Func<string, int, int, int>));
return method.CreateDelegate<JumpTableDelegate>();
}

// Internal for testing
Expand Down Expand Up @@ -79,27 +79,22 @@ private static void GenerateMethodBody(
var methods = Methods.Instance;

// Initializing top-level locals - this is similar to...
// ReadOnlySpan<char> span = arg0.AsSpan(arg1, arg2);
// ref byte p = ref Unsafe.As<char, byte>(MemoryMarshal.GetReference<char>(span))
// var length = span.Length;
il.Emit(OpCodes.Ldarga_S, 0);
il.Emit(OpCodes.Call, methods.SpanLength);
il.Emit(OpCodes.Stloc, locals.Length);

// arg0.AsSpan(arg1, arg2)
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Ldarg_1);
il.Emit(OpCodes.Ldarg_2);
il.Emit(OpCodes.Call, methods.AsSpan);

// ReadOnlySpan<char> = ...
il.Emit(OpCodes.Stloc, locals.Span);
// ref byte p = ref Unsafe.As<char, byte>(MemoryMarshal.GetReference<char>(arg0))

// MemoryMarshal.GetReference<char>(span)
il.Emit(OpCodes.Ldloc, locals.Span);
// MemoryMarshal.GetReference<char>(arg0)
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Call, methods.GetReference);

// Unsafe.As<char, byte>(...)
il.Emit(OpCodes.Call, methods.As);

// ref byte p = ...
il.Emit(OpCodes.Stloc_0, locals.P);
il.Emit(OpCodes.Stloc, locals.P);

var groups = entries.GroupBy(e => e.text.Length).ToArray();
for (var i = 0; i < groups.Length; i++)
Expand All @@ -109,7 +104,7 @@ private static void GenerateMethodBody(
// Similar to 'if (length != X) { ... }
var inside = il.DefineLabel();
var next = il.DefineLabel();
il.Emit(OpCodes.Ldarg_2);
il.Emit(OpCodes.Ldloc, locals.Length);
il.Emit(OpCodes.Ldc_I4, group.Key);
il.Emit(OpCodes.Beq, inside);
il.Emit(OpCodes.Br, next);
Expand Down Expand Up @@ -408,7 +403,7 @@ private static void SaveAssembly(
MethodAttributes.Public | MethodAttributes.Static,
CallingConventions.Standard,
typeof(int),
new [] { typeof(string), typeof(int), typeof(int), };
new [] { typeof(ReadOnlySpan<char>) };

GenerateMethodBody(method.GetILGenerator(), defaultDestination, exitDestination, entries, vectorize);

Expand All @@ -422,7 +417,7 @@ private class Locals
public Locals(ILGenerator il, bool vectorize)
{
P = il.DeclareLocal(typeof(byte).MakeByRefType());
Span = il.DeclareLocal(typeof(ReadOnlySpan<char>));
Length = il.DeclareLocal(typeof(int));

UInt16Value = il.DeclareLocal(typeof(ushort));

Expand Down Expand Up @@ -460,9 +455,9 @@ public Locals(ILGenerator il, bool vectorize)
public LocalBuilder P { get; }

/// <summary>
/// Holds the relevant portion of the path as a Span[byte].
/// Holds the length of the Span
/// </summary>
public LocalBuilder Span { get; }
public LocalBuilder Length { get; }
}

private class Labels
Expand All @@ -486,6 +481,8 @@ private class Methods

private Methods()
{
SpanLength = typeof(Span<char>).GetProperty(nameof(Span<char>.Length)).GetGetMethod();

// Can't use GetMethod because the parameter is a generic method parameters.
Add = typeof(Unsafe)
.GetMethods(BindingFlags.Public | BindingFlags.Static)
Expand Down Expand Up @@ -578,6 +575,9 @@ private Methods()
/// </summary>
public MethodInfo AsSpan { get; }


public MethodInfo SpanLength { get; }

/// <summary>
/// <see cref="MemoryMarshal.GetReference{T}(ReadOnlySpan{T})"/> - GetReference[char]
/// </summary>
Expand Down
20 changes: 10 additions & 10 deletions src/Http/Routing/src/Matching/ILEmitTrieJumpTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -30,7 +30,7 @@ internal class ILEmitTrieJumpTable : JumpTable
// Will be replaced at runtime by the generated code.
//
// Internal for testing
internal Func<string, PathSegment, int> _getDestination;
internal JumpTableDelegate _getDestination;

public ILEmitTrieJumpTable(
int defaultDestination,
Expand All @@ -48,14 +48,14 @@ public ILEmitTrieJumpTable(
_getDestination = FallbackGetDestination;
}

public override int GetDestination(string path, PathSegment segment)
public override int GetDestination(ReadOnlySpan<char> path)
{
return _getDestination(path, segment);
return _getDestination(path);
}

// Used when we haven't yet initialized the IL trie. We defer compilation of the IL for startup
// performance.
private int FallbackGetDestination(string path, PathSegment segment)
private int FallbackGetDestination(ReadOnlySpan<char> path)
{
if (path.Length == 0)
{
Expand All @@ -65,7 +65,7 @@ private int FallbackGetDestination(string path, PathSegment segment)
// We only hit this code path if the IL delegate is still initializing.
LazyInitializer.EnsureInitialized(ref _task, ref _initializing, ref _lock, InitializeILDelegateAsync);

return _fallback.GetDestination(path, segment);
return _fallback.GetDestination(path);
}

// Internal for testing
Expand All @@ -82,17 +82,17 @@ await Task.Run(() =>
internal void InitializeILDelegate()
{
var generated = ILEmitTrieFactory.Create(_defaultDestination, _exitDestination, _entries, _vectorize);
_getDestination = (string path, PathSegment segment) =>
_getDestination = (ReadOnlySpan<char> path) =>
{
if (segment.Length == 0)
if (path.Length == 0)
{
return _exitDestination;
}

var result = generated(path, segment.Start, segment.Length);
var result = generated(path);
if (result == ILEmitTrieFactory.NotAscii)
{
result = _fallback.GetDestination(path, segment);
result = _fallback.GetDestination(path);
}

return result;
Expand Down
7 changes: 5 additions & 2 deletions src/Http/Routing/src/Matching/JumpTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Diagnostics;

namespace Microsoft.AspNetCore.Routing.Matching
{
internal delegate int JumpTableDelegate(ReadOnlySpan<char> path);

[DebuggerDisplay("{DebuggerToString(),nq}")]
internal abstract class JumpTable
{
public abstract int GetDestination(string path, PathSegment segment);
public abstract int GetDestination(ReadOnlySpan<char> path);

public virtual string DebuggerToString()
{
Expand Down
Loading