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

Simplify goroutine setup for async task requests #4924

Conversation

mehulkar
Copy link
Contributor

@mehulkar mehulkar commented May 12, 2023

This change works, but I see that the total execution time jumps from ~500ms to ~2s though (both FULL TURBO). I can share the test I'm using (in front).

The execution time shouldn't change by much, because execution is closed off before the wg.Wait(), so I don't think this diff is the best either

@vercel
Copy link

vercel bot commented May 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-basic-web 🔄 Building (Inspect) May 13, 2023 5:01am
examples-cra-web 🔄 Building (Inspect) May 13, 2023 5:01am
examples-nonmonorepo 🔄 Building (Inspect) May 13, 2023 5:01am
examples-tailwind-web 🔄 Building (Inspect) May 13, 2023 5:01am
examples-vite-web 🔄 Building (Inspect) May 13, 2023 5:01am
6 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) May 13, 2023 5:01am
examples-gatsby-web ⬜️ Ignored (Inspect) May 13, 2023 5:01am
examples-kitchensink-blog ⬜️ Ignored (Inspect) May 13, 2023 5:01am
examples-native-web ⬜️ Ignored (Inspect) May 13, 2023 5:01am
examples-svelte-web ⬜️ Ignored (Inspect) May 13, 2023 5:01am
turbo-site ⬜️ Ignored (Inspect) Visit Preview May 13, 2023 5:01am

@mehulkar mehulkar requested a review from gsoltis May 12, 2023 22:48
@mehulkar mehulkar marked this pull request as ready for review May 12, 2023 22:48
@mehulkar mehulkar requested a review from a team as a code owner May 12, 2023 22:48
@mehulkar mehulkar requested review from tknickman and removed request for a team May 12, 2023 22:48
@github-actions
Copy link
Contributor

github-actions bot commented May 12, 2023

🟢 CI successful 🟢

Thanks

@gsoltis
Copy link
Contributor

gsoltis commented May 13, 2023

Not quite what I meant. You're seeing a slowdown because the channel is unbuffered: writers must wait for an available reader. While the first request is pending, no one can write to the channel. So, the goal is to read from the channel as fast as possible and not block reading on IO. Pseudocode of one way to do it:

func start() {
  firstRequestDone := make(chan interface{})
	go func() {
		pending := []*request{}
		startedFirstRequest := false
	FirstRequest:
		for {
			select {
			case <-firstRequestDone:
				for _, req := range pending {
					go doReq(req)
				}
				break FirstRequest
			case req <- requestsChannel:
				if startedFirstRequest {
					pending = append(pending, req)
				} else {
					startedFirstRequest = true
					doFirstReq(req)
				}
			}
		}
		for _, req := range requestsChannel {
			go doReq(req)
		}
	}()
}

In this case, you have a single reader that never blocks. When it gets the first request, it starts it. While the first request is in flight, it buffers subsequent requests. When the first request completes, it fires off the buffered requests and switches to reading and immediately firing off requests

@mehulkar mehulkar force-pushed the mk/refactor-goroutines branch from 0507314 to 4c2a970 Compare May 13, 2023 04:44
// listen for new requests coming in
case req := <-c.requests:
if req == nil {
continue
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this check to prevent a bunch of nils getting added to the pending slice, but I'm not sure why/how nil messages are being passed to this channel. My guess is it has something to do with the for loop, but I'm not sure.

@mehulkar
Copy link
Contributor Author

@gsoltis thanks! this mostly worked. Only thing I don't understand he requests channel receives a ton of nil messages and not sure where they're coming from.

I also don't quite get the for+select combo pattern, but I see it's a common way to listen for messages, so I'll read up on that.

@mehulkar
Copy link
Contributor Author

Going to merge this into the base PR for a holistic look and put in any questions there.

@mehulkar mehulkar merged commit 0159de9 into mehulkar/turbo-916-runs-send-task-summaries-as-tasks-finish May 15, 2023
@mehulkar mehulkar deleted the mk/refactor-goroutines branch May 15, 2023 17:25
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.

2 participants