-
Notifications
You must be signed in to change notification settings - Fork 9
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
#2382: ccm-lb: add backoff to avoid performance issues w/locking #2383
base: develop
Are you sure you want to change the base?
Conversation
2ea8a29
to
ccb3bb1
Compare
This might need a more complete treatment with a increasing backoff, but for now this code is running well. |
023b907
to
1889b34
Compare
e7bd01a
to
900f9aa
Compare
Optional('lb_iterations'): [ | ||
{ | ||
'id': int, | ||
'tasks': [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct. I wonder if it is possible to avoid repetition by having task
defined separately 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this might work:
Task = {
'entity': And({
Optional('collection_id'): int,
'home': int,
Optional('id'): int,
Optional('seq_id'): int,
Optional('index'): [int],
'type': str,
'migratable': bool,
Optional('objgroup_id'): int
}, validate_ids),
'node': int,
'resource': str,
Optional('subphases'): [
{
'id': int,
'time': float,
}
],
'time': float,
Optional('user_defined'): dict,
Optional('attributes'): dict
}
'tasks': [
Task,
],
auto info = makeClusterSummary(id); | ||
cur_clusters_.emplace(id, std::move(info)); | ||
|
||
//computeClusterSummary(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented out code.
); | ||
} | ||
|
||
auto iter = try_locks_.begin(); | ||
auto lock = *iter; | ||
try_locks_.erase(iter); | ||
|
||
if (lock.forced_release) { | ||
std::this_thread::sleep_for(std::chrono::microseconds(100)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number: put a comment why 100
is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Needs a rebase too.
For any potential reviewer: using Hide whitespace
helps a lot 😉
Fixes #2382