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

Custom pool executor was missing priority and stacksize options #2921

Merged
merged 2 commits into from Oct 4, 2017

Conversation

biddisco
Copy link
Contributor

@biddisco biddisco commented Oct 1, 2017

The problem reported in #2914 can actually be resolved by using a custom pool executor on the "default" pool instead of a default executor, for the tasks that do not go to the original custom pool.

Doing this reveald that the custom pool executor was missing the correct logic to handle stacksize and priority options that we need.

@biddisco
Copy link
Contributor Author

biddisco commented Oct 1, 2017

NB. I do not like the name customized_pool_executor and I think we should rename it early - before it starts to get used in examples and by users in the wider community

Instead of customized_pool_executor I suggest

  1. custom_pool_executor (better English in my view)
  2. pool_executor (since this is what it is, if existing classes use this name, we should adjust them accordingly)
  3. please suggest something.

@hkaiser
Copy link
Member

hkaiser commented Oct 1, 2017

pool_executor is fine.

@sithhell
Copy link
Member

sithhell commented Oct 1, 2017 via email

@biddisco
Copy link
Contributor Author

biddisco commented Oct 1, 2017

ok. I will merge this commit (once approved) and then do a new one with the new name of pool_executorin the next day or two.

@hkaiser
Copy link
Member

hkaiser commented Oct 1, 2017

Let's change the name right away. If it's a matter of a day or two - no problem.

@biddisco
Copy link
Contributor Author

biddisco commented Oct 1, 2017

OK. executor renamed. I've got several other PR's on the way that depend on this, so I'm keen to get this merged ASAP. (= before tutorial!)

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

@hkaiser
Copy link
Member

hkaiser commented Oct 1, 2017

Let's merge this as soon as the tests have cycled.

@biddisco
Copy link
Contributor Author

biddisco commented Oct 2, 2017

hmm. The appveyor test failed for this, but I cannot find anything wrong.

@hkaiser
Copy link
Member

hkaiser commented Oct 2, 2017

For some reason, hello_world prints:

[00:41:36] python build\bin\hpxrun.py -l2 -t2 build\Debug\bin\hello_world.exe -- --hpx:bind=none
[00:42:33] hello world from OS-thread 1 on locality 0
[00:42:33] hello world from OS-thread 1 on locality 0
[00:42:33] hello world from OS-thread 0 on locality 0
[00:42:34] hello world from OS-thread 0 on locality 1
[00:42:34] hello world from OS-thread 1 on locality 1

(which is clearly wrong) and hangs afterwards. I'm not sure if this is caused by your changes, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants