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

Mutable Job implementation #286

Merged
merged 1 commit into from Oct 19, 2016

Conversation

@ig-sinicyn
Copy link
Collaborator

ig-sinicyn commented Oct 19, 2016

As proposed in #279.

Before:

    var job = Job.Dry
       .WithLaunchCount(1)
       .WithWarmupCount(1)
       .WithTargetCount(targetCount)
       .WithGcForce(false));       

After:

    var job = new Job(Job.Dry)
    {
        Run = { LaunchCount = 1, WarmupCount = 1, TargetCount = targetCount },
        Env = { Gc = { Force = false } }
    }.Freeze();

The property system is inspired by WPF dependency properties + freezable objects approach.
However, there are restrictions made to maintain compatibility with the current codebase:

  • Each property (Characteristic) can be used only once per object instance (no way to assign different values for Job.Id and Job.EnvMode.Id).
  • The value of the properties with type derived from JobMode (e.g. job.EnvMode) cannot be null.

The expected use-case pattern is

  • create
  • set properties (via obj initializer syntax or using the .Apply() method)
  • freeze and reuse.

Frozen objects are immutable and therefore can be shared across threads or stored in static fields.
Jobs are frozen automatically when added into the config, so the `did we forget to freeze the job?' check is unnecessary.

P.S. Tests are green, suggestions are welcome:)

@AndreyAkinshin AndreyAkinshin added this to the v0.10.0 milestone Oct 19, 2016
@AndreyAkinshin AndreyAkinshin self-assigned this Oct 19, 2016
@AndreyAkinshin AndreyAkinshin merged commit 2596471 into dotnet:master Oct 19, 2016
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@adamsitnik

This comment has been minimized.

Copy link
Member

adamsitnik commented Oct 20, 2016

@ig-sinicyn I really appreciate the amount of hard work you have done to implement it but personally I don't like the new syntax. Some of my arguments:

  1. Some of our users are used to fluent api, now it's gone and the backward compatibility is broken. If they want to upgrade to the new version for their existing benchmarks they have to learn new syntax and change existing code.
  2. With the old, "fluent" api it was an implementation detail covered by us how properties of the Job were named. For example Job.With(Runtime.Clr) was leaving it to us how we save this state, with new Job() { Env = { Runtime = Runtime.Core } } we are not allowed to rename Env or any other public properties if we want to keep backward compatibility for future versions.
  3. The goal was to simplify the code, but the size of it has doubled (731 lines removed, 1618 added)

@AndreyAkinshin I think that we should keep the backward compatibility with the fluent api and only extend it by new, optional "object initializer" syntax. We should follow OCP

@ig-sinicyn

This comment has been minimized.

Copy link
Collaborator Author

ig-sinicyn commented Oct 20, 2016

@adamsitnik , see your point.

At first, there's a Job .ctor that allows you to pass multiple job mutators, e.g.

var job = new Job("MyJob", EnvMode.Clr, RunMode.Dry, GcMode.Server);

and it's very easy to create your own ones.
The obj initializer syntax is fallback for case there's no predefined mutator or you do want to set all properties explicitly.

However, I do agree that breaking changes are bad and will do another PR with WithXyz() fluent extensions coming back. Or we can just revert this PR, either way will do : )

P.S.

The goal was to simplify the code, but the size of it has doubled (731 lines removed, 1618 added)

Yep, looks like a fail ; )
OK, seriously, the goal was to simplify the public API, not the implementation. Obviously there's a room for making it better, so any ideas / suggestions / fixes are welcome!

@AndreyAkinshin

This comment has been minimized.

Copy link
Member

AndreyAkinshin commented Oct 20, 2016

Support of the WithXXX() methods in v0.10.0 looks like a good idea. @ig-sinicyn, I'm waiting for another PR.
In general, I will care a lot about backward compatibility since v1.0.0. Right now it's ok to do breaking changes, if it improves the API. I like new approach to specify jobs and I don't know which approach (old or new) is better. Let's keep them both and try to use it.
What about the internal implementation, this PR is a step forward. A lot of ugly methods were rewritten, the architecture has become more flexible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.