Based on patches by Eric Pouech and Torge Matthies.
Signed-off-by: Jacek Caban jacek@codeweavers.com --- Both Eric and Torge sent patches for this and both used ConsoleFlags for passing the information. While reviewing both series, I was wondering if using ConsoleHandle instead would be cleaner and I think it is. I ended up having a complete third version...
Eric, Torge, I hope you will review this.
dlls/kernel32/tests/console.c | 3 --- dlls/kernelbase/console.c | 21 +++++++++++++++------ dlls/kernelbase/process.c | 11 +++++++---- include/wine/condrv.h | 5 +++-- programs/conhost/conhost.c | 11 ++++++++--- programs/conhost/conhost.h | 1 + 6 files changed, 34 insertions(+), 18 deletions(-)
Le 05/04/2022 à 14:34, Jacek Caban a écrit :
Based on patches by Eric Pouech and Torge Matthies.
Signed-off-by: Jacek Caban jacek@codeweavers.com
Both Eric and Torge sent patches for this and both used ConsoleFlags for passing the information. While reviewing both series, I was wondering if using ConsoleHandle instead would be cleaner and I think it is. I ended up having a complete third version...
Eric, Torge, I hope you will review this.
dlls/kernel32/tests/console.c | 3 --- dlls/kernelbase/console.c | 21 +++++++++++++++------ dlls/kernelbase/process.c | 11 +++++++---- include/wine/condrv.h | 5 +++-- programs/conhost/conhost.c | 11 ++++++++--- programs/conhost/conhost.h | 1 + 6 files changed, 34 insertions(+), 18 deletions(-)
two comments:
- the value for ConsoleHandle in child process for a GUI exec is not consistent depending on passed flags:
+ it's going to be 0 value when CREATE_NO_WINDOW is passed
+ it's value is untouched when 0 is passed (at least none of CREATE_NO_WINDOW, DETACH_PROCESS, CREATE_NEW_CONSOLE)
under windows, it's always zeroed out (as the remaining todo_wine in test_CreateProcessCUI shows)
the attached (modified) patch implements this behavior; but it's quite a change...
- just for the record: I wonder if in dlls/ntdll/unix/process.c we shouldn't also detach from the unix controling terminal in CONSOLE_HANDLE_ALLOC_NO_WINDOW case (but it would be impacting with next patch as ctrl-c would no longer be working). so I'd say no
this 'next' - aka third - patch, just creates a window less console in some cases for an initial process and should fix (or at least improve) a couple of open bugs
(to make it short, in scenario like "wine foo >& log < /dev/null", it prevents foo and the subprocesses when in cui subsystem to create visible consoles)
(just to share it here)
https://bugs.winehq.org/show_bug.cgi?id=52771
On 4/5/22 17:00, Eric Pouech wrote:
Le 05/04/2022 à 14:34, Jacek Caban a écrit :
Based on patches by Eric Pouech and Torge Matthies.
Signed-off-by: Jacek Caban jacek@codeweavers.com
Both Eric and Torge sent patches for this and both used ConsoleFlags for passing the information. While reviewing both series, I was wondering if using ConsoleHandle instead would be cleaner and I think it is. I ended up having a complete third version...
Eric, Torge, I hope you will review this.
dlls/kernel32/tests/console.c | 3 --- dlls/kernelbase/console.c | 21 +++++++++++++++------ dlls/kernelbase/process.c | 11 +++++++---- include/wine/condrv.h | 5 +++-- programs/conhost/conhost.c | 11 ++++++++--- programs/conhost/conhost.h | 1 + 6 files changed, 34 insertions(+), 18 deletions(-)
two comments:
- the value for ConsoleHandle in child process for a GUI exec is not
consistent depending on passed flags:
+ it's going to be 0 value when CREATE_NO_WINDOW is passed
+ it's value is untouched when 0 is passed (at least none of CREATE_NO_WINDOW, DETACH_PROCESS, CREATE_NEW_CONSOLE)
under windows, it's always zeroed out (as the remaining todo_wine in test_CreateProcessCUI shows)
the attached (modified) patch implements this behavior; but it's quite a change...
Yes, it's quite a change, but since we already opened that can of worms, I think we should handle that case properly as well. Handling GUI processes deserves a separated patch, I was hoping that you could follow up with that fix.
- just for the record: I wonder if in dlls/ntdll/unix/process.c we
shouldn't also detach from the unix controling terminal in CONSOLE_HANDLE_ALLOC_NO_WINDOW case (but it would be impacting with next patch as ctrl-c would no longer be working). so I'd say no
Good point, I missed that part.
this 'next' - aka third - patch, just creates a window less console in some cases for an initial process and should fix (or at least improve) a couple of open bugs
(to make it short, in scenario like "wine foo >& log < /dev/null", it prevents foo and the subprocesses when in cui subsystem to create visible consoles)
(just to share it here)
To recap our private conversation, there are nice things about that solution: it makes such invocation more similar to a more common situation on Windows and I like that aspect. My main concern is that it will make invoking Wine in situations when there is no Unix console available much more expensive, because we'd need to create an extra conhost process that would be useless in majority of cases. I know that we have some users using Wine as part of their Unix scripts, likely ran in the background, for whom it's likely to provide worse performance due to need to create twice as many Wine processes. It may not be a deal breaker, but I think it's a concern that we should consider.
One possible solution to that would be, for example, to have something like CONSOLE_HANDLE_SHELL_NO_WINDOW, which would be inherited by CreateProcess(0) and prevent from creating an actual new console. This would achieve most of this patch's goals with lower runtime price.
Other possible solution is more specific to Wine tests, which are causing most of those problems. We could just build tests as GUI applications and avoid the problem in the first place. It's not a general solution, through. We could maybe have wineconsole --no-window option that, instead of using AllocConsole, would use CREATE_NO_WINDOW. This would essentially give user an opt-in option to achieve the same outcome as your patch.
Anyway, none of above is perfect. I just thrown more ideas to the pool, hoping for your and/or other's opinions.
I think that the root of the problem is that there is no single right thing when Wine is invoked by Unix process with no Unix console available (when Unix console is available, the choice to use it is mostly uncontroversial). In some cases, creating a console window is desired, while for others it's not. Currently user is able to force console window creation by running explicitly wineconsole, but the opposite is not possible under assumption that "default" is good for other cases. This is an equivalent of creating the process with DETACHED_PROCESS on Windows.
Thanks,
Jacek
this 'next' - aka third - patch, just creates a window less console in some cases for an initial process and should fix (or at least improve) a couple of open bugs
(to make it short, in scenario like "wine foo >& log < /dev/null", it prevents foo and the subprocesses when in cui subsystem to create visible consoles)
(just to share it here)
To recap our private conversation, there are nice things about that solution: it makes such invocation more similar to a more common situation on Windows and I like that aspect. My main concern is that it will make invoking Wine in situations when there is no Unix console available much more expensive, because we'd need to create an extra conhost process that would be useless in majority of cases. I know that we have some users using Wine as part of their Unix scripts, likely ran in the background, for whom it's likely to provide worse performance due to need to create twice as many Wine processes. It may not be a deal breaker, but I think it's a concern that we should consider.
One possible solution to that would be, for example, to have something like CONSOLE_HANDLE_SHELL_NO_WINDOW, which would be inherited by CreateProcess(0) and prevent from creating an actual new console. This would achieve most of this patch's goals with lower runtime price.
Other possible solution is more specific to Wine tests, which are causing most of those problems. We could just build tests as GUI applications and avoid the problem in the first place. It's not a general solution, through. We could maybe have wineconsole --no-window option that, instead of using AllocConsole, would use CREATE_NO_WINDOW. This would essentially give user an opt-in option to achieve the same outcome as your patch.
Anyway, none of above is perfect. I just thrown more ideas to the pool, hoping for your and/or other's opinions.
I think that the root of the problem is that there is no single right thing when Wine is invoked by Unix process with no Unix console available (when Unix console is available, the choice to use it is mostly uncontroversial). In some cases, creating a console window is desired, while for others it's not. Currently user is able to force console window creation by running explicitly wineconsole, but the opposite is not possible under assumption that "default" is good for other cases. This is an equivalent of creating the process with DETACHED_PROCESS on Windows.
anyway it would be interesting to have a tool (say wineconsole) where we could launch a wine binary with a given console setup (=detached, =window, =no-window...); optinonaly, it should also take care of redirection of unix std handles (and pass them to subchild)... that would require another flag (like --std-inherit). this would allow to have an answer to any specific need
I'm all against moving wine tests to GUI subsystem... it doesn't solve the bottom line issue: windows "regular" behavior is that CUI processes are attached to a console...
yet to decide what's the best option
- for Wine's default for initial process not connected to a unix tty
- for testbot
I'll wait for this to be settled before zero:ing ConsoleHandle for GUI processes...
Le 06/04/2022 à 08:34, Eric Pouech a écrit :
this 'next' - aka third - patch, just creates a window less console in some cases for an initial process and should fix (or at least improve) a couple of open bugs
(to make it short, in scenario like "wine foo >& log < /dev/null", it prevents foo and the subprocesses when in cui subsystem to create visible consoles)
(just to share it here)
To recap our private conversation, there are nice things about that solution: it makes such invocation more similar to a more common situation on Windows and I like that aspect. My main concern is that it will make invoking Wine in situations when there is no Unix console available much more expensive, because we'd need to create an extra conhost process that would be useless in majority of cases. I know that we have some users using Wine as part of their Unix scripts, likely ran in the background, for whom it's likely to provide worse performance due to need to create twice as many Wine processes. It may not be a deal breaker, but I think it's a concern that we should consider.
One possible solution to that would be, for example, to have something like CONSOLE_HANDLE_SHELL_NO_WINDOW, which would be inherited by CreateProcess(0) and prevent from creating an actual new console. This would achieve most of this patch's goals with lower runtime price.
Other possible solution is more specific to Wine tests, which are causing most of those problems. We could just build tests as GUI applications and avoid the problem in the first place. It's not a general solution, through. We could maybe have wineconsole --no-window option that, instead of using AllocConsole, would use CREATE_NO_WINDOW. This would essentially give user an opt-in option to achieve the same outcome as your patch.
Anyway, none of above is perfect. I just thrown more ideas to the pool, hoping for your and/or other's opinions.
I think that the root of the problem is that there is no single right thing when Wine is invoked by Unix process with no Unix console available (when Unix console is available, the choice to use it is mostly uncontroversial). In some cases, creating a console window is desired, while for others it's not. Currently user is able to force console window creation by running explicitly wineconsole, but the opposite is not possible under assumption that "default" is good for other cases. This is an equivalent of creating the process with DETACHED_PROCESS on Windows.
anyway it would be interesting to have a tool (say wineconsole) where we could launch a wine binary with a given console setup (=detached, =window, =no-window...); optinonaly, it should also take care of redirection of unix std handles (and pass them to subchild)... that would require another flag (like --std-inherit). this would allow to have an answer to any specific need
I'm all against moving wine tests to GUI subsystem... it doesn't solve the bottom line issue: windows "regular" behavior is that CUI processes are attached to a console...
yet to decide what's the best option
for Wine's default for initial process not connected to a unix tty
for testbot
I'll wait for this to be settled before zero:ing ConsoleHandle for GUI processes...
that patch might work instead
- at least, with it, testbot seems happier on user32:input (https://testbot.winehq.org/JobDetails.pl?Key=112035&f101=task.log#k101)
- applies on top of the two previous patches
in terms of performance, I measured locally:
4 different settings:
- when starting attached to a unix console
- when starting not attached to a unix console (current wine tree)
- when starting not attached to a unix console (current wine tree + ALLOC_NO_WINDOW for initial process)
- when starting not attached to a unix console (current wine tree + SHELL_NO_WINDOW for initial process = the attached patch)
three measures:
==============
in all measures, using time on running tasklist.exe (which only does a couple of FIXME:s; so a proxy for process startup time - and tear down)
done on bare metal, with a relatively idle machine with "decent" #procs and memory... so values to be seem as best case, heavier work load might increase the differences
measure #1:
starting a new wine session: start done 10 times (waiting in between for wineserver to shut down; with warm up to fill fs caches)
| | /w patch | full session init | |------------+-----------------+-------------------| | Unix shell | | 166.3 ms | | no shell | | 163.7 ms | | no shell | alloc_no_window | 166 ms | | no shell | shell_no_window | 164 ms | roughly, a slight degradation when starting up conhost.exe ; no measurable difference when not attached to a unix console
measure #2:
keeping an active wine session, starting 1000 times tasklist in a row
| | /w patch | incremental start | |------------+-----------------+-------------------| | Unix shell | | 9,440 ms | | no shell | | 7,903 ms | | no shell | alloc_no_window | 9,271 ms | | no shell | shell_no_window | 7,951 ms | so a 15 to 20% impact on process startup... shell_no_window doesn't show measurable difference from current situation for non attached processes
measure #3
same logic as measure #2, using start.exe in the middle
| | /w patch | incremental start | |------------+-----------------+-------------------| | Unix shell | | 38.315 ms | | no shell | | 31.945 ms | | no shell | alloc_no_window | 38.472 ms | | no shell | shell_no_window | 32.428 ms | confirms reasults of measure #2, but with a huge impact of start.exe (path lookup...)
Hi Eric,
On 4/6/22 11:57, Eric Pouech wrote:
anyway it would be interesting to have a tool (say wineconsole) where we could launch a wine binary with a given console setup (=detached, =window, =no-window...); optinonaly, it should also take care of redirection of unix std handles (and pass them to subchild)... that would require another flag (like --std-inherit). this would allow to have an answer to any specific need
Sure, I think that we can be much more flexible about wineconsole itself, so that sounds fine to me.
I'm all against moving wine tests to GUI subsystem... it doesn't solve the bottom line issue: windows "regular" behavior is that CUI processes are attached to a console...
yet to decide what's the best option
for Wine's default for initial process not connected to a unix tty
for testbot
I'll wait for this to be settled before zero:ing ConsoleHandle for GUI processes...
that patch might work instead
- at least, with it, testbot seems happier on user32:input
(https://testbot.winehq.org/JobDetails.pl?Key=112035&f101=task.log#k101)
- applies on top of the two previous patches
in terms of performance, I measured locally:
4 different settings:
when starting attached to a unix console
when starting not attached to a unix console (current wine tree)
when starting not attached to a unix console (current wine tree +
ALLOC_NO_WINDOW for initial process)
- when starting not attached to a unix console (current wine tree +
SHELL_NO_WINDOW for initial process = the attached patch)
three measures:
==============
in all measures, using time on running tasklist.exe (which only does a couple of FIXME:s; so a proxy for process startup time - and tear down)
done on bare metal, with a relatively idle machine with "decent" #procs and memory... so values to be seem as best case, heavier work load might increase the differences
measure #1:
starting a new wine session: start done 10 times (waiting in between for wineserver to shut down; with warm up to fill fs caches)
| | /w patch | full session init | |------------+-----------------+-------------------| | Unix shell | | 166.3 ms | | no shell | | 163.7 ms | | no shell | alloc_no_window | 166 ms | | no shell | shell_no_window | 164 ms | roughly, a slight degradation when starting up conhost.exe ; no measurable difference when not attached to a unix console
measure #2:
keeping an active wine session, starting 1000 times tasklist in a row
| | /w patch | incremental start | |------------+-----------------+-------------------| | Unix shell | | 9,440 ms | | no shell | | 7,903 ms | | no shell | alloc_no_window | 9,271 ms | | no shell | shell_no_window | 7,951 ms | so a 15 to 20% impact on process startup... shell_no_window doesn't show measurable difference from current situation for non attached processes
measure #3
same logic as measure #2, using start.exe in the middle
| | /w patch | incremental start | |------------+-----------------+-------------------| | Unix shell | | 38.315 ms | | no shell | | 31.945 ms | | no shell | alloc_no_window | 38.472 ms | | no shell | shell_no_window | 32.428 ms | confirms reasults of measure #2, but with a huge impact of start.exe (path lookup...)
Thanks, those results are interesting. I must admit that I expected bigger difference (like time x2).
Given how simple the solution that you attached is, I'm biased towards choosing it. If you're happy if that, I'd say let's use it. One note about it, through, new_process server request uses "> 3" check to decide if it should use duplicate_handle() on passed handle, which does not work well with CONSOLE_HANDLE_SHELL_NO_WINDOW being 5. Maybe we should use -1, -2, ... for magic handles instead.
Thanks,
Jacek
Thanks, those results are interesting. I must admit that I expected bigger difference (like time x2).
as usual, tests on one single machine cannot be taken as an overall answer... those should be extended on a more loaded machine
Given how simple the solution that you attached is, I'm biased towards choosing it. If you're happy if that, I'd say let's use it.
well let's hope it will not trigger errors from process trying to access this pseudo handle for real... I'll update the patch to
1) correctly handle the pseudo value in FreeConsole (maybe other places too)
2) add a FIXME in console_ioctl when using it so that we can detect those potential cases
One note about it, through, new_process server request uses "> 3" check to decide if it should use duplicate_handle() on passed handle, which does not work well with CONSOLE_HANDLE_SHELL_NO_WINDOW being 5. Maybe we should use -1, -2, ... for magic handles instead.
good catch
(need retesting as this zeroes out the ConsoleHandle value in child :-(, and need perhaps to fake this case in the tests somehow...)
well the -1, -2 might also collide with the "magic" handle values (server/handle.c:get_magic_handle()), not sure if it's the best thing
since it's only for ConsoleHandle inheritance, instead of > 3, we'd better test that handle isn't a multiple of four and is between 1 and 5 (even add an helper in condrv.h to test for that)
I'll resend the whole serie then when it's done
-
Eric Pouech -
Jacek Caban