Skip to content

Conversation

@JanProvaznik
Copy link
Member

Reverts #12356

#12356 (comment)
regresses exception counts in VS RPS

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reverts a previous change that avoided spawning conhost processes for worker nodes on Windows. The revert is necessary because the original optimization caused regressions in exception counts during Visual Studio RPS (Requests Per Second) testing.

  • Removes the DETACHED_PROCESS flag from worker node creation on Windows
  • Removes the associated constant definition and explanatory comment

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Build/BackEnd/Node/NativeMethods.cs Removes the DETACHED_PROCESS constant definition
src/Build/BackEnd/Components/Communications/NodeLauncher.cs Removes the code that sets the DETACHED_PROCESS flag and related comment

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@YuliiaKovalova
Copy link
Member

Can we attempt to fix it in the worker node?
In Xmake add this method

[DllImport("kernel32.dll")]
private static extern IntPtr GetConsoleWindow();

private static bool HasConsole()
{
    return GetConsoleWindow() != IntPtr.Zero;
}

and invoke it in SetConsoleUI before making any console operations?

@rainersigwald
Copy link
Member

@YuliiaKovalova yes but I'm going to hold myself to the "revert then fix" baseline.

@JanProvaznik JanProvaznik enabled auto-merge (squash) August 20, 2025 13:10
@JanProvaznik JanProvaznik self-assigned this Aug 20, 2025
@JanProvaznik JanProvaznik merged commit 7ac6a85 into main Aug 20, 2025
9 checks passed
@JanProvaznik JanProvaznik deleted the revert-12356-detached branch August 20, 2025 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants