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

Single point of full config creation #565

Merged
merged 2 commits into from Oct 15, 2017

Conversation

Projects
None yet
4 participants
@ig-sinicyn
Collaborator

ig-sinicyn commented Oct 8, 2017

This commit tries to solve the issue of full config creation.
Some parts of current code create full configs three times during benchmark run, others do not create full config at all.

The fix is following:

  • A new type, BenchmarkRunInfo is introduced. It keeps together benchmarks, a full config and a type for which the config was created. In case when there are multiple types in a single run (like with join switch) the current behavior is preserved: config is created using first type.
  • XxxToBenchmarks() methods now return BenchmarkRunInfo, runner methods that accept BenchmarkRunInfo DO NOT create full config, all others use full config that returned from XxxToBenchmarks() call.

As a bonus I've added a ReadOnlyConfig class that allows to prevent errors like "passing a partial config to methods that expect full one". The class may be removed if not needed.

P.S. The build fails due to 9076a69 commit, branch build is fine, https://ci.appveyor.com/project/ig-sinicyn/benchmarkdotnet/build/0.10.9.47

{
var target = b.Target;
return new Benchmark(
new Target(target.Type, target.Method, target.GlobalSetupMethod, target.GlobalCleanupMethod,

This comment has been minimized.

@ig-sinicyn

ig-sinicyn Oct 8, 2017

Collaborator

I'm not sure if the new Target() part is required at all, code left as is.

@ig-sinicyn

ig-sinicyn Oct 8, 2017

Collaborator

I'm not sure if the new Target() part is required at all, code left as is.

@adamsitnik

I like the idea of ReadOnlyConfig but I am not sure if the fact that we created config few times was a problem. On the other hand you want to change a contract of public methods here.

@AndreyAkinshin what do you think? I am not sure if the gain is big enough to merge it.

@ig-sinicyn could you create an issue next time before creating the PR?

Show outdated Hide outdated src/BenchmarkDotNet.Core/Configs/ReadOnlyConfig.cs Outdated
public static Summary Run(BenchmarkRunInfo[] benchmarkRunInfos, Func<Job, IToolchain> toolchainProvider)
{
var temp = benchmarkRunInfos.FirstOrDefault();
var benchmarkRunInfo = new BenchmarkRunInfo(

This comment has been minimized.

@adamsitnik

adamsitnik Oct 15, 2017

Member

I don't understand this idea.

You have an array of infos, each may have different Type and Config. But before you run all of them you choose the type and config of the first info.

What if every info has it's own config and/or different type? Could you explain?

@adamsitnik

adamsitnik Oct 15, 2017

Member

I don't understand this idea.

You have an array of infos, each may have different Type and Config. But before you run all of them you choose the type and config of the first info.

What if every info has it's own config and/or different type? Could you explain?

This comment has been minimized.

@ig-sinicyn

ig-sinicyn Oct 15, 2017

Collaborator

@adamsitnik I dislike it too but I cannot imagine anything better. Current code preserves behavior from Benchmark switcher with --join switch and BenchmarkConverter's BenchmarksFromSource().

@ig-sinicyn

ig-sinicyn Oct 15, 2017

Collaborator

@adamsitnik I dislike it too but I cannot imagine anything better. Current code preserves behavior from Benchmark switcher with --join switch and BenchmarkConverter's BenchmarksFromSource().

Show outdated Hide outdated src/BenchmarkDotNet.Core/Running/ClassicBenchmarkConverter.cs Outdated
BenchmarkRunnerCore.Run(benchmarks, config, ToolchainExtensions.GetToolchain);
public static Summary Run(Benchmark[] benchmarks, IConfig config)
{
var targetType = benchmarks?.FirstOrDefault()?.Target.Type;

This comment has been minimized.

@adamsitnik

adamsitnik Oct 15, 2017

Member

again: why do you assume that all Benchmarks have the same Target?

@adamsitnik

adamsitnik Oct 15, 2017

Member

again: why do you assume that all Benchmarks have the same Target?

Show outdated Hide outdated src/BenchmarkDotNet/Running/BenchmarkRunner.cs Outdated
@ig-sinicyn

This comment has been minimized.

Show comment
Hide comment
@ig-sinicyn

ig-sinicyn Oct 15, 2017

Collaborator

@adamsitnik
Pushed the fixes:)

I am not sure if the fact that we created config few times was a problem. On the other hand you want to change a contract of public methods here.

Well, there is a problem with custom config attributes. Multiple calls of the same attribute will add multiple instances of logger/exporter/diagnoser etc into resulting config and there's no code that removes duplicates from resulting code. This may be fixed by altering the GetFullConfig so this is not a strong argument, definitely:)
Also, with current code there is no way to pass custom config into BenchmarkRunnerCore as is, without altering it via config attributes.

Technically, both of these may be fixed without changing the public API. But without explicit API contract it is very easy to broke fixed behavior accidentally, just by calling overload that calls GetFullConfig(fullConfig) or forgetting to call it on partial config.

could you create an issue next time before creating the PR?

Ok.

Collaborator

ig-sinicyn commented Oct 15, 2017

@adamsitnik
Pushed the fixes:)

I am not sure if the fact that we created config few times was a problem. On the other hand you want to change a contract of public methods here.

Well, there is a problem with custom config attributes. Multiple calls of the same attribute will add multiple instances of logger/exporter/diagnoser etc into resulting config and there's no code that removes duplicates from resulting code. This may be fixed by altering the GetFullConfig so this is not a strong argument, definitely:)
Also, with current code there is no way to pass custom config into BenchmarkRunnerCore as is, without altering it via config attributes.

Technically, both of these may be fixed without changing the public API. But without explicit API contract it is very easy to broke fixed behavior accidentally, just by calling overload that calls GetFullConfig(fullConfig) or forgetting to call it on partial config.

could you create an issue next time before creating the PR?

Ok.

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Oct 15, 2017

Member

@ig-sinicyn big thanks for the explanation and fixes!

s. Multiple calls of the same attribute will add multiple instances of logger/exporter/diagnoser etc into resulting config and there's no code that removes duplicates from resulting code.

now I get it! Does it mean that this PR closes #360 ?

Member

adamsitnik commented Oct 15, 2017

@ig-sinicyn big thanks for the explanation and fixes!

s. Multiple calls of the same attribute will add multiple instances of logger/exporter/diagnoser etc into resulting config and there's no code that removes duplicates from resulting code.

now I get it! Does it mean that this PR closes #360 ?

@ig-sinicyn

This comment has been minimized.

Show comment
Hide comment
@ig-sinicyn

ig-sinicyn Oct 15, 2017

Collaborator

@adamsitnik
Well, almost:)
There still be duplicates if there are multiple attributes that do add same exporters/diagnosers/etc but at least there's now is a single point (the GetFullConfig() method) where all duplicates may be removed.

I did not fix it as I'm not sure how to implement duplicate detection. Should all items expose smth like Id property or it will be better to implement IEquatable on base types?

Collaborator

ig-sinicyn commented Oct 15, 2017

@adamsitnik
Well, almost:)
There still be duplicates if there are multiple attributes that do add same exporters/diagnosers/etc but at least there's now is a single point (the GetFullConfig() method) where all duplicates may be removed.

I did not fix it as I'm not sure how to implement duplicate detection. Should all items expose smth like Id property or it will be better to implement IEquatable on base types?

@adamsitnik adamsitnik merged commit de45ad9 into dotnet:master Oct 15, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@AndreyAkinshin AndreyAkinshin added this to the v0.10.10 milestone Oct 29, 2017

alinasmirnova added a commit to alinasmirnova/BenchmarkDotNet that referenced this pull request Sep 22, 2018

Single point of full config creation (dotnet#565)
* Single point of full config creation (force rebuild)

* Code cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment