Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Adding Span IndexOf that takes an index and count#16787

Closed
ahsonkhan wants to merge 2 commits into
dotnet:masterfrom
ahsonkhan:SpanIndexAPI
Closed

Adding Span IndexOf that takes an index and count#16787
ahsonkhan wants to merge 2 commits into
dotnet:masterfrom
ahsonkhan:SpanIndexAPI

Conversation

@ahsonkhan
Copy link
Copy Markdown

No description provided.

@ahsonkhan
Copy link
Copy Markdown
Author

cc @KrzysztofCwalina, @davidfowl

@@ -87,6 +87,7 @@ public static class SpanExtensions
{
public static int IndexOf<T>(this Span<T> span, T value) where T:struct, IEquatable<T> { throw null; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where is the approved API proposal for this? What about the other overloads?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is just a temporary hack until the Span codegen is fixed, you should keep it in corefxlab.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I agree corfxlab might be a better place for it. corfx APIs should represent what we are confident in shipping.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's fine, will slicing + indexof ever be as fast as passing an index and count? I have some doubts about that.

while (span.Length > 0)
{
      int index = span.IndexOf((byte)'\n');
      if (index == -1) break;
      var line = span.Slice(0, index);
      span = span.Slice(index + 1);
}

VS

int index = 0;
while (index < span.Length)
{
      int lineIndex = span.IndexOf((byte)'\n', index, span.Length - index);
      if (lineIndex == -1) break;
      var line = span.Slice(index, lineIndex - index + 1);
      index += length.Length
}

Will that ever be within the ballpark?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it should be pretty equivalent - once the JIT team implements the planned struct enregistration that handles Span well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wow, I look forward to seeing this. @ahsonkhan Can you make sure we have a benchmark for this scenario? It's pretty fundamental.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have created it in the PR in corefxlab:

result = bytes.IndexOf(lookupVal, startIndex, count);

VS.

result = bytes.Slice(startIndex, count).IndexOf(lookupVal);

return -1;
}

public static int IndexOf(ref byte searchSpace, byte value, int startIndex, int count, int spanLength)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do not think it makes sense to replicate the worker method for this - you should use the existing public static int IndexOf(ref byte searchSpace, byte value, int length) method.

@ahsonkhan
Copy link
Copy Markdown
Author

This PR can be closed. Opened one in corefxlab - dotnet/corefxlab#1278

@jkotas jkotas closed this Mar 7, 2017
@karelz karelz modified the milestone: 2.0.0 Mar 10, 2017
@ahsonkhan ahsonkhan deleted the SpanIndexAPI branch March 25, 2017 01:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants