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

feat: run deal filters in parallel #1746

Merged
merged 6 commits into from
Oct 18, 2023
Merged

feat: run deal filters in parallel #1746

merged 6 commits into from
Oct 18, 2023

Conversation

LexLuthr
Copy link
Collaborator

@LexLuthr LexLuthr commented Oct 11, 2023

Fixes #1733
This PR moves processesOnlineDeals and processOfflineDeals to their own goroutines.
Possible side effects:

  1. Multiple CMD execution of custom filters in parallel (High CPU utilisation depending on the number of deal being processed)
  2. The deal rejection will not be in order anymore. Say, we get deal A first and deal B later and start processing them. It is possible for deal A to be rejected for lack of resources and deal B to be accepted depending upon which goroutine finishes first.

DO NOT MERGE

@LexLuthr
Copy link
Collaborator Author

LexLuthr commented Oct 11, 2023

Tested on devnet:
Sent 1000 deals in a for loop from bash with no sleep time.

real    15m16.104s
user    3m48.002s
sys     2m34.272s

Not a single deal timed out. Every deal was responded to correctly by either accepting it or rejecting it.

Screenshot 2023-10-11 at 2 35 32 PM

@LexLuthr LexLuthr requested review from nonsense and masih October 11, 2023 10:40
// set up deal handler so that clients can subscribe to deal update events
dh, err := p.mkAndInsertDealHandler(deal.DealUuid)
if err != nil {
sendErrorResp(&acceptError{error: err, isSevereError: true, reason: "server error: starting deal thread"})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sendErrorResp(&acceptError{error: err, isSevereError: true, reason: "server error: starting deal thread"})
sendErrorResp(&acceptError{error: err, isSevereError: true, reason: "server error: setting up deal handler"})

sendErrorResp(&acceptError{error: err, isSevereError: true, reason: "server error: starting deal thread"})
continue
// send an accept response
dealReq.rsp <- acceptDealResp{&api.ProviderDealRejectionInfo{Accepted: true}, nil}
Copy link
Member

Choose a reason for hiding this comment

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

Specify keys in struct and avoid setting zero value fields (i.e. err)?

Suggested change
dealReq.rsp <- acceptDealResp{&api.ProviderDealRejectionInfo{Accepted: true}, nil}
dealReq.rsp <- acceptDealResp{ri: &api.ProviderDealRejectionInfo{Accepted: true}}

}

// send an accept response
processedDeal.rsp <- acceptDealResp{&api.ProviderDealRejectionInfo{Accepted: true}, nil}
Copy link
Member

Choose a reason for hiding this comment

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

  • Ditto; see previous comment.
  • Can we do something about duplicate code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have refactored to reduce the duplicate code and fix the RSP channel responses with specific keys.

@LexLuthr LexLuthr requested a review from masih October 11, 2023 12:10
@nonsense
Copy link
Member

@LexLuthr I see you posted numbers on how this perform with the new code (this PR). Could you also post numbers on how the code performs on current main, i.e. without these changes? Not necessarily 1000 deals if that's too slow, but some numbers for us to understand the impact of this PR. Thanks!

@LexLuthr
Copy link
Collaborator Author

@nonsense I don't see much difference when there is no external filter running.

Main
real 0m53.846s
user 0m26.219s
sys 0m13.832s

Current branch
real 0m53.415s
user 0m26.272s
sys 0m12.452s

I have asked @cryptowhizzard to test it on his miner and check if there are any real world improvement in timeout rate.

We probably need to work with CIDGravity to move to a different execution mode than os.Exec to speed up the filter. Preferably an RPC call to a local process.

@LexLuthr
Copy link
Collaborator Author

@cryptowhizzard Can you confirm if you are seeing some improvement with this PR?

@LexLuthr LexLuthr merged commit 24d7303 into main Oct 18, 2023
1 check passed
@LexLuthr LexLuthr deleted the fix/deal-decider branch October 18, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

New incoming offline deals are blocked when boost receives 200+ online deals.
3 participants