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

remove superfluous ABT_thread_yield() call #293

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Conversation

carns
Copy link
Member

@carns carns commented Sep 17, 2024

improves ops/s by 2-3% in sequntial benchmarks

@carns carns self-assigned this Sep 17, 2024
@mdorier
Copy link
Contributor

mdorier commented Sep 17, 2024

Careful here; this ABT_thread_yield was added to solve this: #278

@carns
Copy link
Member Author

carns commented Sep 17, 2024

Ah, shoot. I keep forgetting about the desire to support the use case where execution streams are mapped to multiple pools.

I'll take a closer look. If that's the case (that we need to do a blind yield, even if there is nothing to do in the current pool, in case there is another pool to be serviced) then its the other yield call at https://github.com/mochi-hpc/mochi-margo/blob/main/src/margo-core.c#L1984 that needs to be removed.

It doesn't make sense to do a yield and then a few lines further down in the function yield again, so one of them needs to go. It sounds like we need to cut the one that has guards around it checking the current pool and go with the one that yields no matter what.

I haven't checked the Argobots code, but for some reason these yields have a measurable cost in low latency operations even if there is nothing to yield to. I must be the cost of pushing the work unit back into the pool (possibly checking other pools depending on the ES configuration) and then popping it back out.

@carns carns force-pushed the carns/dev-reduce-yields branch from 967f205 to 4bee21b Compare September 17, 2024 17:04
@carns
Copy link
Member Author

carns commented Sep 17, 2024

Trying another strategy with this commit (and also more carefully commenting the reasoning for the logic). If this passes CI then I think it will work.

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.86%. Comparing base (5f08e35) to head (9854e4e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #293      +/-   ##
==========================================
+ Coverage   57.80%   57.86%   +0.05%     
==========================================
  Files          70       70              
  Lines       10168    10165       -3     
  Branches     1335     1334       -1     
==========================================
+ Hits         5878     5882       +4     
+ Misses       3456     3451       -5     
+ Partials      834      832       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- eliminates duplicate yield call in favor of just calling the one that
  has the fewest condition restrictions so that we cover more
  configuration cases
- also redo comments to better describe the reasoning behind the logic
@carns carns force-pushed the carns/dev-reduce-yields branch from 4bee21b to 9854e4e Compare September 17, 2024 17:16
@carns
Copy link
Member Author

carns commented Sep 17, 2024

Yeah this variation seems to work fine. Squashing and merging. Thanks for pointing out the problem with the original attempt @mdorier

@carns carns closed this Sep 17, 2024
@carns carns reopened this Sep 17, 2024
@carns carns merged commit c151895 into main Sep 17, 2024
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2024
@carns carns deleted the carns/dev-reduce-yields branch September 17, 2024 17:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants