#14931 closed enhancement (fixed)

wxExecute/wxProcess set sub-process priority/niceness

Reported by: ghostvoodooman Owned by:
Priority: low Milestone: 2.9.5
Component: base Version: stable-latest
Keywords: wxExecute wxProcess priority niceness work-needed Cc:
Blocked By: Blocking:
Patch: yes

Description

I'd like to spawn sub-process with idle/lowest priority class under MSW, it would be nice if it were implemented. On unices it is called niceness, and of course it can be set only to positive number (lower priority) except the process is executed under root privileges. So the method should return some result value to indicate potential error.

Attachments (4)

process_priority.patch download (4.8 KB) - added by ghostvoodooman 21 months ago.
patch #1
process_priority2.patch download (4.6 KB) - added by ghostvoodooman 21 months ago.
removed "id="fixed typo
process_priority3.patch download (7.7 KB) - added by ghostvoodooman 21 months ago.
3rd version
process_priority4.patch download (7.5 KB) - added by ghostvoodooman 21 months ago.
4th version of patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 21 months ago by vadz

  • Keywords simple added
  • Priority changed from normal to low
  • Status changed from new to confirmed

We already have wxThread::SetPriority() so it would make sense to have this method in wxProcess too. Implementing it should be pretty straightforward using CreateProcess() under Windows and nice(2) or setpriority(2) under Unix.

Changed 21 months ago by ghostvoodooman

patch #1

comment:2 Changed 21 months ago by ghostvoodooman

  • Patch set

I have attached proposed patch implementing priority of sub-process. I used wxThread semantics. I have successfully tested it under MSW, but not under Unix (I don't have suitable Linux box at the hand ATM).

Changed 21 months ago by ghostvoodooman

removed "id="fixed typo

comment:3 Changed 21 months ago by ghostvoodooman

I have removed my leftover "id=" assignment, and a typo in error message in patch process_priority2.patch .

comment:4 Changed 21 months ago by vadz

  • Keywords work-needed added; simple removed

Thanks, looks mostly good, except for one thing I don't understand ad a few minor remarks:

  1. The thing I don't understand is where are the new wxProcess methods implemented, I don't see them in the patch at all, have you forgotten to include src/common/process.cpp in the diff?
  2. Related to this, wxProcess::GetPriority() doesn't seem to be very useful if it returns just the priority you had set before. It would probably be more useful to have static wxProcess::GetPriority(int pid).
  3. I think it would be better to add a new enum containing wxPRIORITY_XXX values, define WXTHREAD_XXX_PRIORITY as wxPRIORITY_XXX and use these values for processes too because I don't see any advantage in having 2 sets of these constants with the same values and meaning.
  4. The documentation needs "@since 2.9.5" added.
  5. I think saying "the following priorities are defined" is slightly misleading, as it may seem that other values can't be used. I'd use "the following symbolic constants can be used in addition to raw values in 0..100 range".
  6. setpriority() is POSIX so normally should be available on the other systems too, but we probably need a configure test for it. It should be as simple as adding AC_CHECK_FUNC(setpriority) to configure.in, HAVE_SETPRIORITY to setup.h.in and testing for this symbol in the code.
  7. THREAD_PRIORITY_NORMAL in Unix code probably won't compile.
  8. The translation of values from 0..100 to -20..19 range doesn't look correct to me because you use integer division for it. Unfortunately I think we've actually defined wx priority values wrongly, it's difficult to map 101 values in our range to 40 values in Unix priority range, so the expression 20-(2*prio)/5 used in src/unix/threadpsx.cpp is wrong too as it gives +20 for 100 instead of the correct +19. We probably should still use this expression though but saturate +20 to +19 (in both places).
  9. setpriority() returns 0 on success and -1 on error so you don't need to reset and test errno explicitly with it, this is only necessary with getpriority() (see the man page).
  10. We have wxLogSysError() for logging the error message with errno value and the corresponding error message, so no need to pass errno to wxLogError() directly.
  11. You don't need to pass getpid() to setpriority() directly, just 0 would do. I don't know if there is any (dis)advantage to using getpid() instead though to be honest.

Changed 21 months ago by ghostvoodooman

3rd version

comment:5 Changed 21 months ago by ghostvoodooman

Replying to vadz:

Thanks, looks mostly good, except for one thing I don't understand ad a few minor remarks:

  1. The thing I don't understand is where are the new wxProcess methods implemented, I don't see them in the patch at all, have you forgotten to include src/common/process.cpp in the diff?

Oh, my err.

  1. Related to this, wxProcess::GetPriority() doesn't seem to be very useful if it returns just the priority you had set before. It would probably be more useful to have static wxProcess::GetPriority(int pid).

I wrote GetPriority() as an accessor for ::wxExecute, since wxProcess::m_priority is in protected section. Now I have removed this function from documentation, since it is inteded for use in the implementation only.

  1. I think it would be better to add a new enum containing wxPRIORITY_XXX values, define WXTHREAD_XXX_PRIORITY as wxPRIORITY_XXX and use these values for processes too because I don't see any advantage in having 2 sets of these constants with the same values and meaning.

done, though I have used enum to map older constant names to new names (I don't think usage of preprocessor's #define should be used in this case). So I decided to move new enum to "wx/defs.h" since it is used in 2 headers (process.h and thread.h").

  1. The documentation needs "@since 2.9.5" added.

done.

  1. I think saying "the following priorities are defined" is slightly misleading, as it may seem that other values can't be used. I'd use "the following symbolic constants can be used in addition to raw values in 0..100 range".

I copied it from wxThread documentation. The new patch is fixing wxThread as well.

  1. setpriority() is POSIX so normally should be available on the other systems too, but we probably need a configure test for it. It should be as simple as adding AC_CHECK_FUNC(setpriority) to configure.in, HAVE_SETPRIORITY to setup.h.in and testing for this symbol in the code.

done. please, regenerate configure script on my behalf.

  1. THREAD_PRIORITY_NORMAL in Unix code probably won't compile.

fixed.

  1. The translation of values from 0..100 to -20..19 range doesn't look correct to me because you use integer division for it. Unfortunately I think we've actually defined wx priority values wrongly, it's difficult to map 101 values in our range to 40 values in Unix priority range, so the expression 20-(2*prio)/5 used in src/unix/threadpsx.cpp is wrong too as it gives +20 for 100 instead of the correct +19. We probably should still use this expression though but saturate +20 to +19 (in both places).

hmmm, do you have a better idea for correct expression? I'm not sure about this.

  1. setpriority() returns 0 on success and -1 on error so you don't need to reset and test errno explicitly with it, this is only necessary with getpriority() (see the man page).

oh, I see, I misinterpreted the man page.

  1. We have wxLogSysError() for logging the error message with errno value and the corresponding error message, so no need to pass errno to wxLogError() directly.

done.

  1. You don't need to pass getpid() to setpriority() directly, just 0 would do. I don't know if there is any (dis)advantage to using getpid() instead though to be honest.

The man page is a bit confusing to me, but I fixed that.

Attaching process_priority3.patch . Does it look better?

Changed 21 months ago by ghostvoodooman

4th version of patch

comment:6 Changed 21 months ago by ghostvoodooman

I fixed comments, and accidental of one line from the heading of file, see the latest patch.

comment:7 follow-up: Changed 21 months ago by ghostvoodooman

Vadim: ping? I know this one is "low priority"... But I am so impatient.

comment:8 in reply to: ↑ 7 Changed 20 months ago by ghostvoodooman

Replying to ghostvoodooman:

Vadim: ping? I know this one is "low priority"... But I am so impatient.

Ping again? I wish this to be applied, as my application needs this. The application consumes nearly 80% of quad-core CPU, and needs low priority for sub-process to not disturb real-time sound processing that consumes a lot of CPU time. So, the sub-process needs to be ran in lowest priority.

Can this patch be applied, or do you have some notes to it?

comment:9 Changed 20 months ago by vadz

  • Milestone set to 2.9.5

This looks good to me, thanks. I'll try applying it soon but I just don't have any time at all right now.

comment:10 Changed 20 months ago by ghostvoodooman

Vadim, nevermind your time aspects, I really appreciate the fact you will apply it. No need to hurry, it is just okay on my side. the only thing that would make me feel unconformable, is the fact it would not get property "milestone set to 2.9.5"

thank you

comment:11 Changed 20 months ago by VZ

(In [73405]) Rename WXTHREAD_XXX_PRIORITY yo wxPRIORITY_XXX.

This will allow to reuse the same constants for the process priorities in an
upcoming commit.

See #14931.

comment:12 Changed 20 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [73406]) Add wxProcess::SetPriority() to allow setting the priority of child processes.

This uses the same conventions as wxThread::SetPriority() but works on the
entire process.

Closes #14931.

comment:13 Changed 20 months ago by VZ

(In [73425]) Fix crash in wxExecute() introduced by r73406.

Don't dereference potentially NULL wxProcess pointer.

See #14931.

comment:14 Changed 20 months ago by VZ

(In [73426]) Fix a crash in wxExecute() in wxMSW too.

Don't dereference potentially NULL wxProcess pointer unconditionally.

This should have been together with the changes of r73425, see #14931.

comment:15 Changed 20 months ago by ghostvoodooman

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version set to 2.9-svn

ICC 13.0.1 emits warning

..\..\src\common\process.cpp(182): warning #186: pointless comparison of unsigned integer with zero
        wxCHECK_RET( priority >= wxPRIORITY_MIN && priority <= wxPRIORITY_MAX,
        ^

since priority is of type unsigned and wxPRIORITY_MIN is declared as enum value 0u.

comment:16 Changed 20 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [73456]) Avoid harmless warning about comparing unsigned with 0.

Fix warning in assert in wxProcess::SetPriority(): don't compare unsigned
priority with wxPRIORITY_MIN which is just 0, the condition is always true.

Closes #14931.

Note: See TracTickets for help on using tickets.