current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite 
let reply =  ref ""

let doAuto  =  
    async {
        let! resultAA =
            async { return SqlCmd.aaHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultBB =
            async { return SqlCmd.bbHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultCC =
            async { return SqlCmd.ccHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultDD =
            async { return SqlCmd.ddHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultEE =
            async { return SqlCmd.ddHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultFF =
            async { return SqlCmd.ffHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultGG =
            async { return SqlCmd.ggHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultHH =
            async { return SqlCmd.hhHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultII =
            async { return SqlCmd.iiHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultJJ =
            async { return SqlCmd.jjHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultKK =
            async { return SqlCmd.kkHits query |> Async.RunSynchronously |> List.ofSeq }

        let length1 = resultAA.Length + resultBB.Length

        if length1 > 9 then reply := "hey " + length1.ToString()
        else
            let x = resultCC.Length + resultDD.Length + resultEE.Length + resultFF.Length + resultGG.Length 
                        + resultHH.Length + resultII.Length + resultJJ.Length + resultKK.Length + length1
            reply := x.ToString()

    } |> Async.StartImmediate

!reply

The typical SqlCmd.xxHits (in module SqlCmd) built using FSharp.Data.SqlCommandTypeProvider looks like this:

[<Literal>]
let sqlXX = "SELECT ...@startsWith..."

type QueryXX = SqlCommand<sqlXX, ConnectionStringName=...>

let xxHits query = 
    let cmdXxHits = QueryXX(connectionString = connectionString, startsWith = query)
    cmdXxHits.AsyncExecute()

I'm pretty inexperienced at using async. Looking for feedback, especially in a couple of areas:

  1. I couldn't think of a better way to get my results out from inside the outer async than the ref. From what I read it sounded like Async.StartImmediate was the best solution for something running in an IIS application.

  2. From the results of the first 2 inner asyncs, I may choose to quit before waiting for any more completions. Should I really be cancelling the remaining asyncs? Does it matter?

share|improve this question
add comment

2 Answers

up vote 1 down vote

Avoid Async.RunSynchronously until absolutely necessary. It is very inefficient by design. Looks like you need:

async { let! xs = SqlCmd.xxxHits query
        return List.ofSeq xs }

Which you should probably abstract into a function.

Once you remove the blocking (RunSynchronously), the ref use also becomes invalid - you are querying the ref potentially before the async changes it - race condition. Try returning it (return) in the main async instead.

If you really want the subqueries to execute in parallel, consider a helper combinator like Async.Parallel rather than starting multiple sub-computations manually.

Your function should return Async<string> instead of string. If the caller of your code wants a string and cannot possibly use Async, they'll call Async.RunSynchronously - make it their problem.

Concerning choosing to quit based on the result of first two asyncs, not sure what you need exactly. Cancellation is an optimization - it may be tricky to get right but may make the code better performing. It's your decision based on the problem domain whether it is worth it or not.

share|improve this answer
    
Yes I want to execute in parallel, but I want to evaluate the first 2 as soon as they are ready, at which point either quit or await the remaining queries. Maybe this is too impractical and I should just execute all in parallel. Another issue I have with Async.Parallel is telling results apart. –  Jack Fox Oct 27 '13 at 22:44
    
You can execute the first 2 in parallel, and then the rest in parallel, with these two steps sequential. You might want some helper combinators like this: gist.github.com/toyvo/20f10e29b09688a08138 –  toyvo Oct 27 '13 at 23:02
add comment
up vote 1 down vote

You're running all the Asyncs through Async.RunSynchronously, which means you shouldn't be running them Async at all. You should be using Execute instead of AsyncExecute in xxHits, and all those Async.RunSynchronously disappear. The let! means that you're still waiting for each before going to the next, even if you are inside of an async. let! in this context just means wait for this async before continuing with the rest of this computation.

The ref lets you (try to) read the result before it is done. It should be replaced with a return. return means that the 'T in your Async<'T> is your result, and you can not get at it before it is done. One less source of bugs.

Given xxHits is:

let xxHits query = 
    let cmdXxHits = QueryXX(connectionString = connectionString, startsWith = query)
    cmdXxHits.Execute()

your main body becomes something like

let doAuto  =  
    let! resultAA = SqlCmd.aaHits query |> List.ofSeq
    let! resultBB = SqlCmd.bbHits query |> List.ofSeq
    ...        

    let length1 = resultAA.Length + resultBB.Length

    if length1 > 9 then reply := "hey " + length1.ToString()
    else
        let x = resultCC.Length + resultDD.Length + resultEE.Length + resultFF.Length + resultGG.Length 
                    + resultHH.Length + resultII.Length + resultJJ.Length + resultKK.Length + length1
        x.ToString()

As it is, you have a perf hit for running async computations that are not async.

share|improve this answer
    
I simplified the queries, the real ones each return something quite different. I'm trying do them in parallel (but I think Async.Parallel will not work for me) and evaluate the first 2 as soon as they are available. –  Jack Fox Oct 27 '13 at 22:53
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.