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

Framework settings in Jobs #194

Closed
mattwarren opened this issue May 31, 2016 · 13 comments
Closed

Framework settings in Jobs #194

mattwarren opened this issue May 31, 2016 · 13 comments
Assignees
Milestone

Comments

@mattwarren
Copy link
Contributor

I'm not sure what the expected behaviour is supposed to be, but I don't think we're handling the Framework setting in the right way.

For instance, if you write a Config like so, would you expect it to run your benchmarks twice, once under .NET 4.5.2 and once under .NET 4.6?

private class Config : ManualConfig
{
    public Config()
    {
        Add(new Job { Framework = Framework.V452 });
        Add(new Job { Framework = Framework.V46 });
                ....
    }
}

Well it doesn't and I'm not sure if it's even possible? Since .NET 4.0 all the updates have been in-place, i.e they install on-top of each other. AFAIK I know if you have .NET 4.6 installed, there's not a way to tell it behave like .NET 4.5 at runtime. The only setting you seem to have is <supportedRuntime version="<version>"/>, but that doesn't seem to offer what we'd need.

What we do change is the <TargetFrameworkVersion> setting in the Program.csproj we generate. This affects what happens at compile-time, but only effects the compilation of the scaffolding code that BenchmarkDotNet generates, not any of the user code that is annotated with [Benchmark] as that has already been compiled. To really change the users benchmark code, they would have to change their project settings, which BenchmarkDotNet can't do:

image

I came across this because I wanted to check out the Array.Empty<T> optimisation that the Roslyn compiler added in .NET 4.6 (see some of it's perils and more info here). So I wanted to write some code and then see the difference between the old .NET 4.5.2 behaviour and the optimised .NET 4.6 version.

Thoughts, idea? Am I doing it wrong?

Is this something we even want to support? If not, why do we allow specifying the Framework value, does it even make sense? Especially if we plan to remove .NET 3.5 and only allow .NET 4.0 and upwards.

What's a valid reason for specifying the Framework value in a job?

@ig-sinicyn
Copy link
Contributor

@mattwarren Actually, there is, however it's almost undocumented and it changes not so much.

I'm talking about <supportedRuntime>'s SKU attribute.
It's per-appdomain setting and there's list of what it changes (look for retargeting changes sections).

Bad news: there're some compile-time retargeting changes (to be honest I can remember only the one with Array.Empty<T>). And there're runtime changes you cannot toggle at all.

So, I'm second here. Don't see any reason to keep the Framework option at all.

As a sidenote. I'd propose to enable validators that can be run in same process the benchmark is run. It will allow to detect cases when environment does not match job settings.
Something like this, I guess.

Offtopic: what about #190? Still waiting for comments before doing PR (there were minor changes but the code is pretty much the same):)

@mattwarren
Copy link
Contributor Author

I'm talking about 's SKU attribute.
It's per-appdomain setting and there's list of what it changes (look for retargeting changes sections).

Hmm, seems I misunderstood what <supportedRuntime> actually does. Although as you say, it's still limited, i.e it only changes the behaviour of "Retargeting changes" (am I understanding correctly?)

As a sidenote. I'd propose to enable validators that can be run in same process the benchmark is run. It will allow to detect cases when environment does not match job settings.

Yeah that would be nice, the more validators the merrier!! I've been saved from producing bad benchmark results several times, thanks to JitOptimizationsValidator.FailOnError

Offtopic: what about #190? Still waiting for comments before doing PR (there were minor changes but the code is pretty much the same):)

Sorry, I completely forgot about that, I'll take a look now.

@ig-sinicyn
Copy link
Contributor

ig-sinicyn commented Jun 6, 2016

i.e it only changes the behaviour of "Retargeting changes" (am I understanding correctly?)

Yep. At least as far as I know. Here's example of settable compatibility switches.

Yeah that would be nice, the more validators the merrier!!

Ok. Will fill an issue.

@mattwarren
Copy link
Contributor Author

Yep. At least as far as I know. Here's example of settable compatibility switches.

Oh I see, so it's not a runtime-wide settings, i.e. "Enable all 4.5.1 comparability switches" (when running under 4.5.1 for example), it's per feature, i.e. "NetFx40_TimeSpanLegacyFormatMode" or "NetFx40_LegacySecurityPolicy".

@ig-sinicyn
Copy link
Contributor

@mattwarren as far as I can remember these switches are also toggled on or off depending on SKU. Could not find a good proof as I've looked at the code a year or two ago.

@mattwarren
Copy link
Contributor Author

@AndreyAkinshin @adamsitnik have you got any objections to the Framework setting being removed? (see the top of this thread for the reasons)

@AndreyAkinshin
Copy link
Member

No objections.

@adamsitnik
Copy link
Member

No objections.

Same here. The less settings the better! I think that we should make it [Obsolete] first to not break people's code.

@mattwarren mattwarren self-assigned this Jun 6, 2016
@mattwarren
Copy link
Contributor Author

thanks, I'll go ahead and make the changes

@adamsitnik
Copy link
Member

@mattwarren I have removed it just to make sure we don't release new version with settings that can affect nothing (after moving to Roslyn it's irrelevant)

@mattwarren
Copy link
Contributor Author

@adamsitnik thanks for doing that

I tried to do it myself the other week but found that Framework was being used by the Toolchain code and so I left it till the Roslyn work was done.

@Tornhoof
Copy link
Contributor

You should probably remove it from documentation too: https://perfdotnet.github.io/BenchmarkDotNet/Configuration/Jobs.htm

@AndreyAkinshin
Copy link
Member

@Tornhoof, Thanks for the notice. However, in the next version of BenchmarkDotNet, we have a lot of improvements in the API and we have completely rewritten documentations for Jobs: https://github.com/PerfDotNet/BenchmarkDotNet/blob/master/docs/guide/Configs/Jobs.md

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

5 participants