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.

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

Improve granularity of Perf.Utf8Encoding.cs tests #1512

Open
GrabYourPitchforks opened this issue Sep 9, 2020 · 3 comments
Open

Improve granularity of Perf.Utf8Encoding.cs tests #1512

GrabYourPitchforks opened this issue Sep 9, 2020 · 3 comments

Comments

@GrabYourPitchforks
Copy link
Member

Per the magic decoder ring at dotnet/runtime#41699 (comment), some of the tests in Perf.Utf8Encoding.cs may be testing too many concepts, which can complicate finding regressions.

I'd recommend reworking this file so that it contains a total of six tests:

  1. A test which calls Encoding.UTF8.GetByteCount(string), which exercises only Utf16Utility.GetPointerToFirstInvalidChar (validation).
  2. A test which calls Encoding.UTF8.GetCharCount(byte[]), which exercises only Utf8Utility.GetPointerToFirstInvalidByte (validation).
  3. A test which calls Encoding.UTF8.GetChars(ROS<byte>, Span<char>), which exercises only Utf8Utility.TranscodeToUtf16 (transcoding).
  4. A test which calls Encoding.UTF8.GetBytes(ROS<char>, Span<byte>), which exercises only Utf8Utility.TranscodeToUtf8 (transcoding).
  5. A test which calls Encoding.UTF8.GetBytes(string); which exercises validation, byte array instantiation, and transcoding.
  6. A test which calls Encoding.UTF8.GetString(byte[]), which exercises validation, string instantiation, and transcoding.

For each of these tests, we should use Encoding.UTF8.Xyz, allowing the JIT's full devirtualization features to kick in.

/cc @adamsitnik @jeffhandley @kunalspathak @carlossanlop @pgovind @tannergooding

@GrabYourPitchforks
Copy link
Member Author

FWIW, I'm willing to do the work for this, but I'd like a little guidance. Will adding or removing methods from this type interfere with current perf investigations?

@carlossanlop
Copy link
Member

I suggest you submit a PR, and we can wait to merge it until @adamsitnik gives the approval.

@adamsitnik
Copy link
Member

Will adding or removing methods from this type interfere with current perf investigations?

As long as there is at least one benchmark that uses the code path that has initially regressed in 5.0 and can be run for 3.1 it should not be a problem.

Thank you for taking perf investigations into account! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants