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

Pipeline iteration builder #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Sanrag
Copy link
Contributor

@Sanrag Sanrag commented Sep 24, 2013

This allows for PipelineIteration instances to be constructed in a much more flexible way making it a general use pipeline framework.

This pull request is based on #17 and will go after it. Please only review Sanrag@71dcda9 for this pull request.

Make PipelineIteration less dependent on the specifics of the stage it
is executing.
There are small number of TODOs which would be cleaned up in a
subsequent change
This allows for PipelineIteration instances to be constructed in a much more flexible way making it a general use pipeline framework.
@matttproud
Copy link
Contributor

@Sanrag, I will give this a full review in a few days (trying to enjoy a post-employment computer detoxification), but I want to leave you with some interim thoughts:

import com.google.inject.*;

@Singleton
public class Main {
    // Guice creates an implicit Provider<T> for its injectables!  How nifty!
    private final Provider<Builder> builders;

    @Inject
    public Main(final Provider<Builder> builders) {
        this.builders = builders;
    }

    private void run() {
        final Builder builder = builder();

        System.out.println("Got builder " + builder);

        final Builder another = builder();

        System.out.println("Got another builder " + another);
    }

    public Builder builder() {
        return builders.get();
    }

    public static void main(final String... unused) {
        final Main main = Guice.createInjector(new Module()).getInstance(Main.class);

        main.run();
    }

    // NOOP module to demonstrate that Guice will do much of this for you—for better or for worse—
    // automagically.
    private static class Module extends AbstractModule {
        @Override
        protected void configure() {}
    }

    // Hard to know what happens with the scoping here.  Give it a try if you want to restrict one of
    // these suckers to a more constrained scope.
    private static class Builder {
        private final SingletonDependency singleton;

        @Inject
        public Builder(final SingletonDependency singleton) {
            this.singleton = singleton;
        }

        @Override
        public String toString() {
            return "Builder{" +
                    "singleton=" + singleton + " " +
                    "id=" + Integer.toHexString(System.identityHashCode(this)) +
                    '}';
        }
    }

    @Singleton
    public static class SingletonDependency {
        @Override
        public String toString() {
            return "SingletonDependency{" +
                    "id=" + Integer.toHexString(System.identityHashCode(this)) +
                    '}';
        }
    }

    // For your own tests …
    public static class ScopedDependency {
    }
}

I think you can effectively get away with the same behavior without passing around or exposing the injector to parts of the stack that ought not be aware of it. Most well-formed Guice implementations hide the injector from as much of the bowels as possible.

The example above yields:

Got builder Builder{singleton=SingletonDependency{id=9a082e2} id=8f0c85e}
Got another builder Builder{singleton=SingletonDependency{id=9a082e2} id=77f297e7}

@matttproud
Copy link
Contributor

@Sanrag, polite ping: Is this code review still alive? :)

@Sanrag
Copy link
Contributor Author

Sanrag commented Mar 26, 2014

We were having discussions around whether we want to make Groningen a general purpose tuning framework (a.k.a. Tunie) or keep it very specific to JVM Tuning. The discussion stretched and I kind of dropped the ball on this. If the decision is to go for latter, I will just go ahead and cancel the pull request.

@etheon
Copy link
Contributor

etheon commented Mar 26, 2014

I kind remember the discussion was whether to replace jvm tuning with a freeform pipeline or abstract the pipeline objects in order to allow the current Pipeline structure to be used as well as allowing the introduction of this freeform type.

My vote being for the latter since there is a significant amount of work that is done based on the nonBuilder style and the Builder style being pretty difficult to enforce any kind of ordering/enumeration/general “this is how the pipeline will behave”ness on...

On Mar 26, 2014, at 8:11 AM, Sanrag Sood [email protected] wrote:

We were having discussions around whether we want to make Groningen a general purpose tuning framework (a.k.a. Tunie) or keep it very specific to JVM Tuning. The discussion stretched and I kind of dropped the ball on this. If the decision is to go for latter, I will just go ahead and cancel the pull request.


Reply to this email directly or view it on GitHub.

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

Successfully merging this pull request may close these issues.

3 participants