3
\$\begingroup\$

I'm beginning to learn about Golang and I would like to have some advice about the following program.

package main

import (
    "fmt"
    "net/http"
    "time"
)

const BenchmarkTry = 1000

type PageBenchmark struct {
    url  string
    time int64 // microseconds
}

func execBenchmark(url string, channel chan PageBenchmark) {
    totalExecTimeChan := make(chan int64, BenchmarkTry) // set size to prevent blocked goroutine
    totalExecTime := int64(0)

    // start all the goroutines
    for i := 0; i < BenchmarkTry; i++ {
        go execHttpRequest(url, totalExecTimeChan)
    }

    // catch new values from totalExecTimeChan values come from execHttpRequest()) and add it to the total
    for i := 0; i < BenchmarkTry; i++ {
        totalExecTime += <-totalExecTimeChan // waiting to get a value from one of the goroutines started in the previous for loop
    }

    channel <- PageBenchmark{url, totalExecTime / BenchmarkTry}
}

// exec http request and attach exec time to channel
func execHttpRequest(url string, channel chan int64) {
    begin := time.Now()
    _, _ = http.Get(url)
    channel <- time.Since(begin).Nanoseconds() / 1000000 // convert to milliseconds
}

func main() {
    sites := [...]string{

    }

    pages := [...]string{

    }

    benchmarkChan := make(chan PageBenchmark, len(sites)*len(pages)) // set size to prevent blocked goroutine

    begin := time.Now()
    fmt.Println("Beginning !")

    // start all the goroutines
    for site := range sites {
        for page := range pages {
            go execBenchmark(sites[site]+pages[page], benchmarkChan)
        }
    }

    // catch new values from benchmarkChan and "print" the PageBenchmark
    for i := 0; i < len(sites)*len(pages); i++ {
        benchmark := <-benchmarkChan
        fmt.Printf("Url : %v\nResponse Time : %d ms\n\n", benchmark.url, benchmark.time)
    }

    // print execution time
    fmt.Println("End.")
    fmt.Println(fmt.Sprintf("%d ms", time.Since(begin).Nanoseconds()/1000000))

}

Basically, I'm making HTTP requests (GET method) to multiple URLs on multiple web sites. 1000 requests by URL in this example.

For now, I just want to start some goroutines in this order:

  1. Main: start a benchmark (goroutine) for each web site pages, get the average execution time and print it.

  2. Page routine: start 1000 goroutines (benchmark tries), get the execution times from a channel and store the average execution time in an other channel.

  3. Execute an HTTP request on the page and store the execution time in a channel.

This piece of code works but there may be things I'm missing.

  • Is the execution scheduling valid?
  • Is it appropriate to use defined size channels here?
  • Is there a better/more effective way to achieve this task?
\$\endgroup\$
3
\$\begingroup\$

When running your program with the Go race detector, it found no race conditions. That's good!

Given this, I would consider your "execution scheduling" valid.

Is it appropriate to use defined-size channels here?

Sure. In this case you make the buffer large enough such that it will not be blocked. You can read more about the difference between buffered and unbuffered channels here.

Benchmark through testing

Is there a better/more effective way to achieve this task?

For benchmarking, it is recommended to use the utilities provided by the testing package.

A lot of this code is boilerplate benchmarking code. With the testing package you can be more confident in the accuracy of your benchmarks, especially since you're using goroutines.

(I will leave switching to the testing package for you.)

Performance bias

sites[site]+pages[page]

Adding strings through the + operator is slow. This could bias your results (ie if you have lots of very long strings), but that also depends on how many sites and pages you test with.

Without knowledge of your full usage, it's hard to tell.

Accuracy

time.Since(begin).Nanoseconds() / 1000000 // convert to milliseconds

I would instead use .Seconds(). Any variation on the scale of milliseconds would be meaningless. From testing with one site and two pages, I received response times of 10288 (10s) and 8128 ms (8s) and an end time of 30073 ms (30s).

Rather than converting to int64 I would keep the type as float64.

String formatting

With the details above:

fmt.Println("End.")
fmt.Println(fmt.Sprintf("%d ms", time.Since(begin).Nanoseconds()/1000000))

Should instead be converted to:

fmt.Printf("End.\nTotal time: %.0fs\n", time.Since(begin).Seconds())

And

fmt.Printf("Url : %v\nResponse Time : %d ms\n\n", benchmark.url, benchmark.time)

Should instead be:

fmt.Printf("Url: %s\nResponse Time: %.0fs\n\n", benchmark.url,
    benchmark.time)

(Notice I use %s rather than %v.)

Variable naming

Variables can have shorter names, especially in shorter functions where their purpose is clear.

  • totalExecTimeChan becomes timeChan
  • totalExecTime becomes time
  • BenchmarkTry becomes tries

I would argue that the names tc and t would be equally fine for the first two.

Capitalization matters in Go. The variable BenchmarkTry and the type PageBenchmark would be exported. In this case it does not make a difference, but for larger programs and packages it would be important.

Miscellaneous

  1. This cast is not needed.

    totalExecTime := int64(0)
    

    With the switch to float64, it can instead be:

    totalExecTime := 0.0
    
  2. Some of your comments extend beyond the 80-character column. It is useful to stay within 80 characters when viewing things split across your screen. Instead, move comments to the line before the code. (Some of these comments are superfluous, but you're learning so that's okay.)

  3. Store len(sites)*len(pages) in a constant, rather than recomputing it each time.

Conclusion

Here is the code I ended up with:

package main

import (
    "fmt"
    "net/http"
    "time"
)

const tries = 1000

type pageBenchmark struct {
    url  string
    time float64
}

func execBenchmark(url string, benchmarks chan pageBenchmark) {
    // prevent blocking goroutine
    timeChan := make(chan float64, tries)
    time := 0.0

    // start all requests
    for i := 0; i < tries; i++ {
        go execHTTPRequest(url, timeChan)
    }

    // catch new values from execHTTPRequest()
    for i := 0; i < tries; i++ {
        // wait to get value from goroutine
        time += <-timeChan
    }

    benchmarks <- pageBenchmark{url, time / tries}
}

// exec HTTP request and attach exec time to channel
func execHTTPRequest(url string, timeChan chan float64) {
    begin := time.Now()
    _, _ = http.Get(url)
    timeChan <- time.Since(begin).Seconds()
}

func main() {
    sites := [...]string{
        // sites
    }

    pages := [...]string{
        // pages
    }

    const length = len(sites) * len(pages)

    // set size to prevent blocked goroutine
    benchmarks := make(chan pageBenchmark, length)

    begin := time.Now()

    fmt.Println("Beginning!\n")

    // start all the goroutines
    for site := range sites {
        for page := range pages {
            go execBenchmark(sites[site]+pages[page], benchmarks)
        }
    }

    // catch and print benchmarks
    for i := 0; i < length; i++ {
        b := <-benchmarks
        fmt.Printf("Url: %s\nResponse Time: %.0fs\n\n", b.url, b.time)
    }

    // print total execution time
    fmt.Printf("End.\nTotal time: %.0fs\n", time.Since(begin).Seconds())
}

Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your time and all the advices :D I've tried the race detector with 2 sites and 12 urls and it's giving me this error : "race: limit on 8128 simultaneously alive goroutines is exceeded, dying". Is that normal? I'll try the testing package too thank you ^^ \$\endgroup\$ – hunomina Mar 21 at 8:35
  • \$\begingroup\$ I tried to use strings.Builder{} to concatenate site and url instead of +=. First try, my computer just crashed... Second try (same code) was very long Don't know why :/ \$\endgroup\$ – hunomina Mar 21 at 9:41
  • \$\begingroup\$ @hunomina I think on this scale, + may not be worth optimizing -- just something to consider. For the race detector error, you receive this because you start 1000 goroutines per execBenchmark call, and you then have 24 calls to execBenchmark -- meaning it tries to spawn 24,000 goroutines. Switch to doing tries non-concurrently -- and only keep calls to execBenchmark concurrent. \$\endgroup\$ – esote Mar 21 at 17:05
  • 1
    \$\begingroup\$ Ok ^^ I may have misunderstood the purpose of the race detector. Let me read it again ;) Finally I'm using the strings.Builder{}, starting the program in a Docker container and it does not crash so happy ending ^^ Thank you for everything @esote ;) \$\endgroup\$ – hunomina Mar 21 at 18:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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