Сreate a tail call diagnoser by 0xGeorgii · Pull Request #573 · dotnet/BenchmarkDotNet

0xGeorgii · 2017-10-30T14:27:10Z

Added diagnoser for JIT Tail Call Events, based on JIT Inlining Events diagnoser.

adamsitnik

@GeorgePlotnikov you have done a great job. However, I am not sure if this enough to cover also F# case. As far as I know, C# compiler does not generate IL opcode for the tail call. But some JITs can do the tail call optimization. This is covered with your example.

However, when it comes to F#, the F# compiler emits the tail call optimization code or simply replaces the code with a simple jump in place(goto the beginning of the method).

Could you also make some research about F# edge cases and cover them? Please let me know if you are interested in doing this.

Sign in to view

+        {
+
+        }
+        /// <summary>


Sign in to view

+            };
+        }
+
+        private bool ShouldPrintEventInfo(Tuple<string, string> namespaces) => !filterByNamespace || namespaces.Item1.StartsWith(expectedNamespace) || namespaces.Item2.StartsWith(expectedNamespace);


Sign in to view

            }
+
+            [Benchmark]
+            public Diagnostics.Windows.TailCallDiagnoser TypeFromCustomDeoendency1()


0xGeorgii · 2017-10-30T20:29:28Z

@adamsitnik thank you very much for your comments, I'll take them into consideration to update the branch. Regarding the F# the case, I'm interested in it, no doubts. If you have some ideas where shall I start the research I would appreciate you a lot.

adamsitnik · 2017-10-30T20:57:03Z

@GeorgePlotnikov this blog post gives a good introduction. However I don't have any link to a post which shows what decisions can be made by the F# compiler.

@dsyme maybe you could provide us some hints for start?

0xGeorgii · 2017-11-07T16:19:32Z

@adamsitnik added F# support and example to the F# sample project with various kinds of recursion calls.

Sign in to view

+        }
+
+        /// <summary>
+        /// creates the new TailCallDiagnoser instace


AndreyAkinshin

@GeorgePlotnikov, could you write an integration test for this diagnoser?

0xGeorgii · 2017-11-16T18:48:40Z

@AndreyAkinshin seems @adamsitnik refused the previously written integration test, because of it was the same as for inline diagnoser, so it was removed by the second commit.

AndreyAkinshin · 2017-11-16T19:38:20Z

@GeorgePlotnikov if I understand correctly, the original test just checked that we can do some basic diagnostics stuff. But we need a test which checks the diagnoser output (with lines like Caller, Callee, Tail prefix, Tail call type, and so on). It will help to verify that diagnoser is still working in case of any changes. Right now I have to check it manually before each release, but it will be better to make it automatic and increase the coverage.
The total test suite duration is still an issue, but we have to find another way to solve it.
@adamsitnik what do you think?

0xGeorgii · 2017-11-16T19:50:04Z

@AndreyAkinshin you mean something like BenchmarkDotNet.IntegrationTests.MemoryDiagnoserTests? I think what you are talking about should look like that. Does it make sense?

adamsitnik · 2017-11-16T20:30:06Z

you mean something like BenchmarkDotNet.IntegrationTests.MemoryDiagnoserTests?

yes, something similar. Run some benchmark, with the new diagnoser and check the output of the diagnoser (info about methods reported to the output).

refused the previously written integration test, because of it was the same as for inline diagnoser

your previous test was just checking if we can have a benchmark, which returns new diagnoser. It was not using the diagnoser. AnyValueCanBeReturned test verifies that we build the right code for all possible use cases of benchmarks (returning different types)

0xGeorgii · 2017-11-16T20:51:22Z

@AndreyAkinshin , @adamsitnik thank you guys for the review. I'll add the test.

0xGeorgii · 2017-11-24T19:46:35Z

@AndreyAkinshin, @adamsitnik hi guys, I've just added the integration test, please review

Sign in to view

+        }
+
+        private IConfig CreateConfig(Jit jit, Platform platform, Runtime runtime, TailCallDiagnoser diagnoser) => ManualConfig.CreateEmpty()
+            .With(Job.ShortRun.With(jit).With(platform).With(runtime))


Sign in to view

+namespace BenchmarkDotNet.IntegrationTests
+{
+#if NET46
+    public class TailCallDiagnoserTests : BenchmarkTestExecutor


AndreyAkinshin · 2017-11-26T10:16:50Z

@GeorgePlotnikov, could you also add some information about this diagnoser in our documentation (the docs folder)? It would be nice to say a few words about what is it, when it can be useful, show a few benchmark examples (e.g. on C# and F#), and show an example of the diagnoser output.

0xGeorgii · 2017-11-30T10:07:40Z

Applied the request review comments to the integration test and pulled the request to update the documentation

* create a tail call diagnoser * added newline at the end of file * updated to support F# * fix proj references * rename local variables for unification * fix typo * added intergration test for the TailCallDiagnoser * chaged directives regarding the project * fixed warnings * refactored * applied the review comments

0xGeorgii added 2 commits October 30, 2017 17:05

create a tail call diagnoser

658a7f2

added newline at the end of file

e13e417

dnfclas added the cla-already-signed label Oct 30, 2017

0xGeorgii mentioned this pull request Oct 30, 2017

Create a tail call diagnoser #273

Closed

adamsitnik requested changes Oct 30, 2017

View reviewed changes

dotnet deleted a comment from dnfclas Nov 1, 2017

jongalloway removed the cla-already-signed label Nov 1, 2017

0xGeorgii added 2 commits November 7, 2017 18:03

updated to support F#

24d0c65

fix proj references

66de265

rename local variables for unification

1301fc3

AndreyAkinshin reviewed Nov 16, 2017

View reviewed changes

Comment thread src/BenchmarkDotNet.Diagnostics.Windows/TailCallDiagnoser.cs Outdated

}

/// <summary>

/// creates the new TailCallDiagnoser instace

This comment was marked as spam.

Sign in to view

AndreyAkinshin requested changes Nov 16, 2017

View reviewed changes

fix typo

8fe17cf

0xGeorgii added 3 commits November 21, 2017 17:27

added intergration test for the TailCallDiagnoser

972ae91

chaged directives regarding the project

5c769e2

fixed warnings

f1151a8

0xGeorgii mentioned this pull request Nov 21, 2017

Broken appveyor build #588

Closed

refactored

fca9939

AndreyAkinshin reviewed Nov 26, 2017

View reviewed changes

applied the review comments

5d30c9e

adamsitnik approved these changes Dec 4, 2017

View reviewed changes

adamsitnik merged commit c1a4b20 into dotnet:master Dec 4, 2017

AndreyAkinshin added this to the v0.10.12 milestone Dec 4, 2017

0xGeorgii deleted the сreate-a-tail-call-diagnoser branch December 4, 2017 22:15

AndyAyersMS mentioned this pull request Jan 27, 2020

The frustrating state of tails calls in .NET dotnet/runtime#2191

Closed

Hidden character warning

Conversation

0xGeorgii commented Oct 30, 2017

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

0xGeorgii commented Oct 30, 2017

Uh oh!

adamsitnik commented Oct 30, 2017

Uh oh!

0xGeorgii commented Nov 7, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

AndreyAkinshin left a comment

Choose a reason for hiding this comment

Uh oh!

0xGeorgii commented Nov 16, 2017

Uh oh!

AndreyAkinshin commented Nov 16, 2017

Uh oh!

0xGeorgii commented Nov 16, 2017

Uh oh!

adamsitnik commented Nov 16, 2017

Uh oh!

0xGeorgii commented Nov 16, 2017

Uh oh!

0xGeorgii commented Nov 24, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

AndreyAkinshin commented Nov 26, 2017

Uh oh!

0xGeorgii commented Nov 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants