scientistproject / Scientist.net

A .NET library for carefully refactoring critical paths. It's a port of GitHub's Ruby Scientist library
MIT License
1.46k stars 95 forks source link

Use and Try blocks run sequentially? #62

Closed ryangribble closed 8 years ago

ryangribble commented 8 years ago

Hey all,

Love the GitHub Engineering blog post on Scientist and just checking out the .Net port of it... great work!

I've succesfully implemented an experiment in a webAPI project where I am trying out a new way to do some calculations whilst still providing users with "the old way" but starting to gather data on any discrepancies between old and new.

Things do "work" however it seems that the "behaviours" are run one at a time, which means (unless im doing something wrong) that running the control Use() and one experimental Try() the api call now takes about twice as long as it used to.

What do you think about having an option to stipulate the number of experiments to run concurrently and then having the internals of ExperimentInstance handle running multiple tasks in parallel? In my case I would be happy to run the Control and single Try at the same time, then hopefully the duration/user impact should be roughly what it always has been, but we start to gain the data that lets us ensure consistent results and then perhaps even start to tune the call duration for better performance.

Or have I just missed something and it's meant to be doing this already?

haacked commented 8 years ago

Interesting challenge. I would imagine that most of the time, we're dealing with code that runs very fast which is why we sequentially randomize the order that the code runs.

I'd be wary of handling concurrency ourselves because the code might not be written for concurrency. Imagine if the method mutates a variable or requires thread context.

Maybe we need a ScienceParallel method that calls a new RunParallel version of Experiment. That way, the fact that experiments and control can run in parallel is explicitly opted in per experiment. I think this is safest because the developer needs to be sure that running the experiment in parallel with the control is a safe thing to do.

haacked commented 8 years ago

There's a discussion on github/scientist about running these in parallel. https://github.com/github/scientist/issues/52

Naturally, this requirement makes it rather complex to run candidates in isolation and in parallel, given that in any “test mode” it’d be needed to pass around (from candidates to the control process) the results or exceptions, and wait for everything to be finished before either returning the appropriate results, or escalating any exceptions.

Fortunately for us, the ability to wait for all and propagate exceptions to the caller is fairly easy given the TPL (aka Tasks).

haacked commented 8 years ago

It occurs to me that if a method under test mutates a variable, the control and experiments also might too. So that's not necessarily a reason to avoid parallelism. The types of code to use Scientist with probably shouldn't mutate things since you'd mutate things multiple times. You'd want to move the mutation out of the code being tested and have the code simply return the correct value.

So in theory, since we store everything as tasks, we might be able to make things concurrent by default and test it out.

ryangribble commented 8 years ago

Yeah I realise various scenarios wouldnt be safe to do this, so this should definitely be an option.

edit: I just read your comment saying maybe this can be the default behaviur even... interesting possibility although from a user perspective as long as i can run things concurrently (wether by default or by choice) i'd be happy

ScienceParallel sounds like a more elegant solution than what I was thinking, which was more just setting something like experiment.ConcurrentTasks = 2 and then having the loop that runs the tasks actually chunk up the tasks by the above integer then run the n Tasks and use Task.WhenAll() to wait for them to finish, before doing the next chunk of n tasks.

In my case there is only 1 control and 1 experiment, they dont mutate anything but are long running (10 - 15s). Im more concerned about establishing "correctness" of a new algorithm rather than comparing execution times... so the main thing for my scenario is in order to put this experiment live without impacting users, I sort of need the parallel execution (otherwise the users ARE impacted with double the response time)

haacked commented 8 years ago

It probably should be ScienceAsync. I bet the primary reason not to be parallel by default is if you're doing some tight synchronous calculation, you're adding overhead by adding concurrency when none is needed.

Maybe the ScienceAsync which is parallel by default is the right approach.

Async expresses the intent that the operation is likely to take some time.

Want to take a crack at it?

ryangribble commented 8 years ago

Want to take a crack at it?

Yep I was already planning on making some changes to let me run stuff in parallel, so I'll see if you guys are happy with what I come up with :grinning: