Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduce cross-process resource management for tasks #5859
Introduce cross-process resource management for tasks #5859
Changes from 52 commits
f48f3d9
bcccfea
73b3466
a8df6ac
963a785
e1a30e6
055116a
10f6eed
aa3f385
57a5c93
9229122
b919834
01953aa
1183e87
0437c32
1bf3017
1ada7c2
e943655
ca6697d
63a324b
09618ec
5a7ad2b
141c57c
007d9f3
6b2be21
5d49876
0486730
2f85924
5e4b035
a9d37a2
43bdec0
479cdfc
9db29bb
e1c3bff
58fa355
a6a9d6d
91fe07a
c9b3c78
ba2db9a
cc60df9
0ab0481
f1e26c3
dfced2f
ed6a404
4545858
3de11c0
608bed7
7a3da6b
d87903f
9a83f35
77344e2
7fffd46
7d8b359
82a25c4
c6d1a8a
b8b52cc
f3e347e
ab384aa
a9d8061
25ec10c
b362600
b830c49
bb2d947
59af842
cacc2ac
3c0a8e7
ded86d5
ad77314
0c0c0b4
a16ca54
1a497d7
7a4e1e0
3d2575c
c416bf5
444ffde
761fb6a
2fb5789
2ce2af8
e3439e4
1653806
6d27a28
df40170
3d03f86
ba264da
d12bd75
1a6326e
0b1a487
d38500b
445ec33
a98adee
15121be
02bce34
b382aab
8dbe055
14c5d3d
aad6f76
3a554b8
63d782e
874ecf3
8ed06ad
97e9f5d
531ee86
27865a8
8016cf9
d10923f
3ac6642
059ac7b
1340f7c
f2381ad
605f6f2
876fef6
f9f0185
719425b
2a6cabe
8b53959
74046a0
93db0c1
de74af6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This section is well-written, but it reads to me as general statement of purpose --> example --> actually, we only care about the example. Although cl.exe was the motivating example, I imagine others will start using it later, so I'd focus on describing this generally rather than diving into a specific example in the first section. On a related note, are you planning to make documentation not in the
specs
folder? If so, this may be a moot point.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.
Now you mention it, is this the right choice? If all tasks opt into this, that would mean you're forcing some of the cores to be idle for no reason.
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.
Where is this sample task? I imagined something like:
// Construct 20x1000 array of data
// Request 20 cores
// Create new Thread[20]
// Only start the first threads
// Join them all, and as they finish, start new ones running.
The last step is a little messy, but it can probably be cleaned up by making callback functions:
Or, even better, something similar using
async
. What do you think?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.
I deleted the sample task because it was no longer useful--but we need one for tests so I'll have to write another.
I don't think it's feasible to provide a single example here. If you're using tasks/the threadpool, you might not want to use this, since the threadpool has its own ideas of resource management (which I believe but can't confirm go cross-process). And the work you'd likely do with dedicated threads is probably fairly different from the work to launch multiple processes. The actual C++ tasks have their own fairly complicated approach that isn't a great example.
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.
Should it always return 1? I imagine users wouldn't use more cores than they're given unless users complain.
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.
I think you should either return 0 (indicating that there is no RM) or min of what is requested and the number of cores, which will be pretty much what we have today without RM.
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.
With the change to blocking/get-at-least-1, I think immediately returning 1 makes the most sense here--and that's what I actually implemented; this doc was stale. Do you think the default for an "enlightened" task should be to overburden the machine, rather than underburden it?
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.
I think returning 1 is confusing as different tasks would do different things with and without RM and they need to know if RM is actually working or not. Maybe we should just add an explicit method for this, like "IsRMSupported", and then it would be not so important what we return here.
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.
I guess it depends on whether we expect users are going to inspect the output before using it. If they intend to immediately use it, returning 1 or min(requested, #cores) makes sense for underloading or overloading the machine respectively. If they're going to inspect it, -1 is easiest as an indicator of "failure" that can't have other meanings like "all cores in use."