Skip to content

Conversation

pakrym
Copy link

@pakrym pakrym commented Dec 22, 2017

We copy paste the same thing into all the repos, just carry it with Microsoft.AspNetCore.BenchmarkRunner.Sources.

Validation config does not include JitOptimizationsValidator.FailOnError so it won't fail in debug anymore.

Also adds support for BeforeMain partial method that is useful for debugging sometimes.

@pakrym pakrym requested a review from pranavkm December 22, 2017 01:04
@pakrym pakrym force-pushed the pakrym/add-common-configs branch from 1f483be to eb84751 Compare December 22, 2017 01:04
private static TextWriter _standardOutput;
private static StringBuilder _standardOutputText;

static partial void BeforeMain(string[] args);

Choose a reason for hiding this comment

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

Nice


namespace BenchmarkDotNet.Attributes
{
public class CoreConfig : ManualConfig

Choose a reason for hiding this comment

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

DefaultConfig?

namespace BenchmarkDotNet.Attributes
{
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Assembly)]
public class AspNetCoreBenchmarkAttribute : Attribute, IConfigSource

Choose a reason for hiding this comment

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

Make these types internal

{
public static bool UseValidationConfig { get; set; }

public Type ConfigType { get;set; }

Choose a reason for hiding this comment

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

Get only (or remove the ctors)

Copy link

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

lgtm

namespace BenchmarkDotNet.Attributes
{
public class CoreConfig : ManualConfig
public class DefaultCoreConfig : ManualConfig

Choose a reason for hiding this comment

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

internal everywhere

@pakrym pakrym force-pushed the pakrym/add-common-configs branch from 92a4744 to 72598ce Compare December 22, 2017 17:38
@pakrym pakrym merged commit 9089ccf into dev Dec 22, 2017
@natemcmaster natemcmaster deleted the pakrym/add-common-configs branch October 25, 2018 00:26
@ghost ghost locked as resolved and limited conversation to collaborators May 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants