Codify the pony(min)threads behavior #3303

alexsnaps · 2019-09-06T01:38:56Z

Draft in order to fix #3152

This won't work… I have many questions already but, wrt the issue at hand:

Should we introduce some value as a magic value for "not provided" (e.g. -1, tho it's currently a uint32_t;
Only allow for --ponynoscale when --ponyminthreads wasn't provided?
What about if --ponyminthreads == --ponythreads and yet --ponynoscale is provided; these two would effectively mean the same thing…

Trying to get the discussion going… I'll then wrap this up (and update the usage and other docs if these exists)

alexsnaps · 2019-09-06T01:39:42Z

src/libponyrt/sched/start.c

@@ -93,6 +96,7 @@ static int parse_opts(int argc, char** argv, options_t* opt)
    {
      case OPT_THREADS: opt->threads = atoi(s.arg_val); break;
      case OPT_MINTHREADS: opt->min_threads = atoi(s.arg_val); break;


If I'm not mistaken this silently overflows… (e.g. if -1 negative values are provided)

alexsnaps · 2019-09-06T01:41:17Z

src/libponyrt/sched/start.c

@@ -148,6 +152,17 @@ PONY_API int pony_init(int argc, char** argv)
    exit(0);
  }

+  if (opt.noscale)


Based on the description on the issue, I think this shouldn't take precedence… yet dunno how I can make sure it actually was explicitly set (with 0 being the default currently). Also what should it do it opt.min_threads == opt.threads, as that'd be the equivalent of opt.noscale…

alexsnaps · 2019-09-06T01:43:12Z

src/libponyrt/sched/start.c

+
+  if (opt.min_threads > opt.threads) 
+  {
+    printf("Can't have --ponyminthreads > --ponythreads (%d > %d)\n", opt.min_threads, opt.threads);


Looks like other errors aren't on stderr, but should they be?
Also is -1 as a return fine?

alexsnaps · 2019-09-06T11:03:49Z

Took a shot at prohibiting --ponyminthreads && --ponynoscale at the same time… Let me know what you think and if that all makes sense, let's update the doc.

alexsnaps · 2019-09-06T11:05:23Z

src/libponyrt/sched/scheduler.c

@@ -1076,7 +1076,7 @@ pony_ctx_t* ponyint_sched_init(uint32_t threads, bool noyield, bool nopin,

  // If minimum thread count is > thread count, cap it at thread count
  if(min_threads > threads)
-    min_threads = threads;
+    min_threads = threads; // this becomes the equivalent of --ponynoscale


Should we remove this entirely now then?

As, with the current implementation, this would be triggered in such a case:

$ ./helloworld --ponyminthreads=2048 Hello, world.

Would it be better to bail out here instead? wdyt?

Here's a proposal: an explicit check against physical cores:

$ ./helloworld --ponyminthreads=2048 Can't have --ponyminthreads > physical cores (2048 > 6)

I think this makes the most sense.

SeanTAllen · 2019-09-07T06:54:35Z

Magic value for what but being provided?

By default minthreads default should be 0.

I think your other questions are answered in the ticket. See #3152 (comment)

alexsnaps · 2019-09-07T11:56:50Z

Should have added a note, I think the latest stuff does that:

$ ./helloworld --ponyminthreads=0 --ponytherads=2
Hello, world.

From sync. We believe there should be a flag for "don't scale my threads".
And --ponyminthreads will error out if it is greater than --ponythreads.

$ ./helloworld --ponyminthreads=20 --ponytherads=2
Can't have --ponyminthreads > --ponythreads (20 > 0)

--ponynoscale is the current idea for turning off scaling.

$ ./helloworld --ponynoscale --ponythreads=20 
Hello, world.
$ ./helloworld --ponynoscale                 
Hello, world.

if --ponynoscale is used on application startup then it should be a startup error to use --ponyminthreads.

$ ./helloworld --ponyminthreads=20 --ponynoscale  
--ponyminthreads & --ponynoscale are mutually exclusive
 $./helloworld --ponyminthreads=20 --ponythreads=20 --ponynoscale
--ponyminthreads & --ponynoscale are mutually exclusive
$ echo $?
255

I just use a bool to see whether --ponyminthreads=0 was set by the user explicitly (as 0 is fine). I think that covers it just fine…

Open questions:

return code? -1 (-> 255) reused the default of the switch statement, but they mean different thing. Should there be or is there a return value (>0) for these kinda errors?
Should we deal with negatives properly? How? Validate the argument was >= 0?

$ ./helloworld --ponyminthreads=-1 --ponythreads=20
Can't have --ponyminthreads > --ponythreads (4294967295 > 20)

Someone may want to review my error messages too… Never feel I do a good job at these!
And… Sean, enjoy your vacation… don't look at this now! ;)

alexsnaps · 2019-09-08T13:16:22Z

Alright, I think that's fairly complete. Added the check for when not specifying --ponythreads as so:

$ ./helloworld --ponyminthreads=2048
Can't have --ponyminthreads > physical cores (2048 > 6)

Let me know what you think!

SeanTAllen · 2019-09-17T16:20:48Z

src/libponyrt/sched/start.c

+  {
+    if (minthreads_set)
+    {
+      printf("--ponyminthreads & --ponynoscale are mutually exclusive\n");


SeanTAllen · 2019-09-17T16:21:46Z

src/libponyrt/sched/start.c

@@ -150,6 +172,12 @@ PONY_API int pony_init(int argc, char** argv)

  ponyint_cpu_init();

+  if (opt.threads == 0 && opt.min_threads > ponyint_cpu_count()) 


This should be added the CHANGELOG.

I'll add it!

SeanTAllen · 2019-09-17T16:29:39Z

Alex this looks pretty good.

Small thing. You are checking to see if minthreads is greater than cores. We discussed and think that that should be if threads is greater than number of cores, it should be disallowed.

Once that is in, do you feel comfortable updating the performance cheat sheet section related to this?

https://www.ponylang.io/reference/pony-performance-cheatsheet/#ponythreads

alexsnaps

Small thing. You are checking to see if minthreads is greater than cores. We discussed and think that that should be if threads is greater than number of cores, it should be disallowed.

So I'd this on top of the current code that checks for --ponyminthreads being larger than the amount of cores, right? The reason I added this is when people specify min threads without specifying --ponythreads. I guess it indeed also makes sense in the latter case tho. So again, currently the code does this:

$ ./helloworld --ponyminthreads=2048
Can't have --ponyminthreads > physical cores (2048 > 6)

I can add this behavior, which I think is indeed just as sensible:

$ ./helloworld --ponythreads=2048
Can't have --ponythreads > physical cores (2048 > 6)

Which I think is what you are asking for… I could also set opt.threads to be the amount of cores in start.c. I'm trying to avoid that this code silently "scales things" down… e.g. if the --ponyminthreads > than the amount of cores and --ponythreads defaulting to the amount of cores being unspecified with the arguments provided while starting up the process...

I'll add this to this PR in any case, but want to make sure we're all in agreement here.

Two questions remain unaddressed:

Should we return -1, i.e 255 as the process' return code on failures related to the runtime config?
Should we add checks for these args not being less than 0 rather than just assign the (signed) int from atoi to an unsigned int field?

Thanks for the review already!

alexsnaps · 2019-09-18T01:32:43Z

src/libponyrt/sched/start.c

@@ -150,6 +172,12 @@ PONY_API int pony_init(int argc, char** argv)

  ponyint_cpu_init();

+  if (opt.threads == 0 && opt.min_threads > ponyint_cpu_count()) 


I'll add it!

SeanTAllen · 2019-09-18T01:52:24Z

I'm not sure of all the permutations of things to check for. Minthreads without setting threads is something I'd never do. But I guess you could do it (although in my mind it would be odd) so I can see how having that check makes sense.

-1 for return exit seems fine to me. Can always be changed later.

Are you asking if there should be checks to make sure someone doesn't do --ponyminthreads=-1 ?

alexsnaps · 2019-09-18T01:57:10Z

I'm not sure of all the permutations of things to check for. Minthreads without setting threads is something I'd never do. But I guess you could do it (although in my mind it would be odd) so I can see how having that check makes sense.

I agree it's not really sensible to me neither, but would it in virtualized environments? Where I don't ever want less than 2 threads, but say for some reason I'm deploying on a single vCPU node? Dunno… nothing prohibits setting --ponyminthreads w/o --ponythreads or should it become a requirement to set both?

In anycase, I think 36d9123 does what you wanted (i.e. check for --ponythreads being <= cores available…

Are you asking if there should be checks to make sure someone doesn't do --ponyminthreads=-1 ?

Yeah, cause currently:

$ ./helloworld --ponythreads=-1
Can't have --ponythreads (the number of threads you'd be running with) > physical cores (4294967295 > 6)

Which might be surprising to some (tho to others, the issue is probably very obvious!)

alexsnaps · 2019-09-18T01:59:06Z

btw, I realize the --ponyminthreads can be clarified at the doc level, i.e. --ponyminthreads=2048 is either the value provided or the --ponythreads, whatever is smaller and where the latter defaults to the number of cores… I can take a shot at trying to clarifying it this way, and get rid of the actual check

SeanTAllen · 2019-09-18T02:05:10Z

I'm not worried about the negative value issue, but checking it isn't less than 0 and reporting on that would be a nice UI improvement. I think that would make sense as a commit unto itself.

SeanTAllen · 2019-09-18T02:07:36Z

btw, I realize the --ponyminthreads can be clarified at the doc level, i.e. --ponyminthreads=2048 is either the value provided or the --ponythreads, whatever is smaller and where the latter defaults to the number of cores… I can take a shot at trying to clarifying it this way, and get rid of the actual check

I didn't follow that. Sorry, can you try saying in another way?

--ponyminthreads=2048 is either the value provided or the --ponythreads, whatever is smaller and where the latter defaults to the number of cores

sounds wrong to me. if ponyminthreads is greater than ponythreads, that should error out. which is not what i think you are saying in the quoted bit.

alexsnaps · 2019-09-18T02:09:02Z

I didn't follow that. Sorry, can you try saying in another way?

I can avoid the explicit check, and let the code in scheduler.c reduce minthreads to threads "silently" (the code I added was trying to avoid silently changing user settings), if you think that's preferable. But in that case I'd be explicit about it in the docs…

sounds wrong to me. if ponyminthreads is greater than ponythreads, that should error out. which is not what i think you are saying in the quoted bit.

Right, but ponythreads is "only" set in scheduler.c in case it was not explicitly specified…

Cool, I'll prepare all of this in a cleaned up commit history and with all the changes discussed.
I'll summarize in another comment here and (hopefully) the doc changes make it all clear in any case. Thanks for the review 🙏 !

(I'll start tonight, but … will probably push it all only tomorrow)

SeanTAllen · 2019-09-18T02:15:40Z

I can avoid the explicit check, and let the code in scheduler.c reduce minthreads to threads "silently" (the code I added was trying to avoid silently changing user settings), if you think that's preferable. But in that case I'd be explicit about it in the docs…

Still not sure I follow. But, I'm in favor of explicit checks and explicit errors as a general rule of thumb.

Right, but ponythreads is "only" set in scheduler.c in case it was not explicitly specified…

Sorry @alexsnaps. I still didn't follow that.

alexsnaps · 2019-09-18T02:18:57Z

Still not sure I follow. But, I'm in favor of explicit checks and explicit errors as a general rule of thumb.

No worries, that last bit:

I'm in favor of explicit checks and explicit errors

Is what I'm arguing for too! So I'll just:

consolidate,
update all the docs,
add a summary to this PR of the changes;

And hopefully we can agree this all makes sense :)
Code speaks best…

As I said on Zulip tho, and as you can now see here, I trust my code more than my English sentences, so please, please, double review the error messages! :)

SeanTAllen · 2019-09-18T02:20:29Z

Sounds good @alexsnaps!

alexsnaps · 2019-09-21T14:06:29Z

Consolidated output & errors on my 6 core CPU:

$ ./helloworld 
Hello, world.

Behavior is unchanged

 $ ./helloworld --ponyminthreads=12
Can't have --ponyminthreads > --ponythreads (12 > 6)

--ponythreads took the default value, i.e. the numbers of physical cores, in this case 6. So we can't have --ponyminthreads be higher than 6

$ ./helloworld --ponyminthreads=4 
Hello, world.

This is fine, given --ponythreads took the default value, i.e. the numbers of physical cores, in this case 6.

 $ ./helloworld --ponyminthreads=4 --ponythreads=6
Hello, world.

Explicit case, all good as --ponythreads is <= physical cores on my machine!

 $ ./helloworld --ponyminthreads=4 --ponythreads=3
Can't have --ponyminthreads > --ponythreads (4 > 3)

Explicit case, but with erronous values provided

$ ./helloworld --ponyminthreads=3 --ponythreads=12
Can't have --ponythreads > physical cores, the number of threads you'd be running with (12 > 6)

Explicit case, but with --ponythreads being larger than the physical cores on the machine.

$ ./helloworld --ponyminthreads=3 --ponythreads=3 --ponynoscale
--ponyminthreads & --ponynoscale are mutually exclusive

Incompatible arguments provided

 $ ./helloworld --ponyminthreads=4 --ponythreads=3 --ponynoscale 
--ponyminthreads & --ponynoscale are mutually exclusive

Explicit case with erronous values and incompatible args. The latter error wins…

 $ ./helloworld --ponythreads=3 --ponynoscale               
Hello, world.

--ponynoscale sets --ponyminthreads to the value provided in --ponythreads, i.e. 3

./helloworld --ponynoscale               
Hello, world.

--ponynoscale sets --ponyminthreads to the implicit value of --ponythreads, i.e. 6 physical cores on my machine.

I'll still:

add the bound check on provided values (atoi signedness issue)
clean up commits
update docs

SeanTAllen · 2019-09-26T00:03:30Z

@alexsnaps let me know when you are ready for another review (and maybe even... a merge!)

alexsnaps · 2019-09-26T00:05:21Z

let me know when you are ready for another review (and maybe even... a merge!)

Wrapping up the doc & unsigned atoi at the airport (and on the flight if need be) right now… I think this should be fine in the next couple of hours…

alexsnaps · 2019-09-26T02:46:51Z

I’m again a disappointment. Didn’t get to finish the unsigned int atoi the way I wanted, so I think it might be best to do this as a separate PR to not hold this one back.

So if you think that behavior sounds good, let’s say this is ready for actual review and possibly a merge. I’ll open the PR against the other repo tomorrow (as I forgot to clone it prior boarding). wdyt?

And sorry about the slow follow up here.

SeanTAllen · 2019-09-26T14:33:39Z

@alexsnaps couple of things before full review.

1- are you comfortable rebasing against master to resolve the CHANGELOG.md conflict?
2- can you add some nice release notes for this change to #3295

SeanTAllen · 2019-09-26T14:34:10Z

@alexsnaps fyi, im planning on reviewing this today or tomorrow. i'd like for it to go out with the 0.32.0 release.

Like so: $ ./helloworld --ponythreads=2048 Can't have --ponythreads > physical cores (2048 > 6)

SeanTAllen · 2019-09-27T11:09:26Z

src/libponyrt/sched/start.c

@@ -107,6 +112,16 @@ static int parse_opts(int argc, char** argv, options_t* opt)
    }
  }

+  if (opt->noscale)
+  {


small nit. this can be fixed in another PR. this { should be on the preceding line.

SeanTAllen · 2019-09-27T11:09:32Z

src/libponyrt/sched/start.c

+  if (opt->noscale)
+  {
+    if (minthreads_set)
+    {


small nit. this can be fixed in another PR. this { should be on the preceding line.

SeanTAllen · 2019-09-27T11:09:50Z

src/libponyrt/sched/start.c

+    opt.threads = ponyint_cpu_count();
+  }
+  else if (opt.threads > ponyint_cpu_count())
+  {


small nit. this can be fixed in another PR. this { should be on the preceding line.

SeanTAllen · 2019-09-27T11:10:05Z

src/libponyrt/sched/start.c

+  }
+
+  if (opt.min_threads > opt.threads)
+  {


small nit. this can be fixed in another PR. this { should be on the preceding line.

That was the style most present in the code base. Not my preference neither… but I try to keep things consistent in a code base, if only one file. If we want { on the same line (my preference 😃 and which is what I understand you are asking for) should I do a single commit formatting pass throughout the file?

SeanTAllen · 2019-09-27T11:11:13Z

@alexsnaps this looks good. there's a few small { related formatting changes, can you do those in another PR? i'm going to merge this as the core of it is solid. Thanks!

alexsnaps commented Sep 6, 2019

View reviewed changes

SeanTAllen reviewed Sep 17, 2019

View reviewed changes

src/libponyrt/sched/start.c

{

if (minthreads_set)

{

printf("--ponyminthreads & --ponynoscale are mutually exclusive\n");

Copy link

Member

SeanTAllen Sep 17, 2019

Choose a reason for hiding this comment

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

nice

SeanTAllen reviewed Sep 17, 2019

View reviewed changes

SeanTAllen mentioned this pull request Sep 17, 2019

--ponyminthreads is confusing #3152

Closed

alexsnaps commented Sep 18, 2019

View reviewed changes

alexsnaps mentioned this pull request Sep 26, 2019

Add --ponynoscale ponylang/ponylang-website#487

Merged

alexsnaps added 2 commits September 26, 2019 11:18

Err out on option ponyminthreads > ponythreads

7b167c5

Add support for --ponynoscale

8941b00

Check --ponythreads <= physical cores

01bb61d

Like so: $ ./helloworld --ponythreads=2048 Can't have --ponythreads > physical cores (2048 > 6)

SeanTAllen reviewed Sep 27, 2019

View reviewed changes

SeanTAllen merged commit cc36d05 into ponylang:master Sep 27, 2019

alexsnaps mentioned this pull request Oct 3, 2019

Avoid silently overflowing (signed) ints to unsigned int values in args #3317

Merged

		@@ -150,6 +172,12 @@ PONY_API int pony_init(int argc, char** argv)

		ponyint_cpu_init();

		if (opt.threads == 0 && opt.min_threads > ponyint_cpu_count())

Codify the pony(min)threads behavior #3303

Codify the pony(min)threads behavior #3303

Conversation

alexsnaps commented Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsnaps commented Sep 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Sep 7, 2019

alexsnaps commented Sep 7, 2019 • edited Loading

alexsnaps commented Sep 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Sep 17, 2019

alexsnaps left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Sep 18, 2019

alexsnaps commented Sep 18, 2019

alexsnaps commented Sep 18, 2019

SeanTAllen commented Sep 18, 2019

SeanTAllen commented Sep 18, 2019

alexsnaps commented Sep 18, 2019 • edited Loading

SeanTAllen commented Sep 18, 2019

alexsnaps commented Sep 18, 2019 • edited Loading

SeanTAllen commented Sep 18, 2019

alexsnaps commented Sep 21, 2019 • edited Loading

Consolidated output & errors on my 6 core CPU:

SeanTAllen commented Sep 26, 2019

alexsnaps commented Sep 26, 2019

alexsnaps commented Sep 26, 2019

SeanTAllen commented Sep 26, 2019

SeanTAllen commented Sep 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Sep 27, 2019

alexsnaps commented Sep 6, 2019 •

edited

Loading

alexsnaps commented Sep 7, 2019 •

edited

Loading

alexsnaps left a comment •

edited

Loading

alexsnaps commented Sep 18, 2019 •

edited

Loading

alexsnaps commented Sep 18, 2019 •

edited

Loading

alexsnaps commented Sep 21, 2019 •

edited

Loading