facebook/sapling
1
1
Fork 0
mirror of https://github.com/facebook/sapling.git synced 2024-10-10 00:45:18 +03:00
Code Issues Projects Releases Wiki Activity

spawn-ext: bump the number of handles we are closing

Browse Source 
Summary:
We got a report about windows in vscode being significantly slower than on
macOs/Linux (https://fburl.com/nv9xwc09) to the point that it can't be
explained by "Windows is just slower". As the post suggests we had a suspicion
it might be related to an issue described in T33479254.

After some debugging I can confirm the following:
1) I can reliably (but not 100% of the time) reproduce the issue on my windows
laptop. I just need to modify a file and do "Quick commit" or "Quick amend" from vscode,
and note that command finished quite fast (just a few seconds, I verified it by
running "hg st" and checking that working copy is now clean, and by running "hg
log -r ." and checking that hash has changed), but vscode takes longer to
notice that (i.e. it keeps spinning and showing that amend is still running).

2) By staring at "Process explorer" I noticed that the problem seem to be in
"hg.exe cloud backup" i.e. when "cloud backup" finishes then vscode stops
spinning. So I suspected it to be a problem.

3) As the next step I started to suspend the process in "Process Explorer"
(note that I started it as admin and SYSTEM user, whatever it means). Then I
looked through file handles in Process explorer and was closing them 1 by 1.
Evetually I noticed that closing handles for named pipes like
"\Device\NamedPipe\uv\0000013E91115170-26220" make vscode stop spinning (it was
usually necessary to close it in hg.exe cloud backup process and all child processes as
well).

4) I also looked at handle value, and noticed that it's bigger than 2048 (0x848).

So currently my suspicion is that the problem is that we don't close enought
handles, and this diff bumps it as a temporary workaround and also walk over handles that multiple of 4 only. quark-zju has an
upstream improvement that would make this hack go away
(https://github.com/rust-lang/rust/pull/75551/commits) but it's not landed yet.
So for now let's try to bump the magic number.

Reviewed By: quark-zju

Differential Revision: D26668773

fbshipit-source-id: ed1c203260a52c3e5449b7b06cf4ecbe4dcf6477
This commit is contained in:
Stanislau Hlebik 2021-02-25 19:56:47 -08:00 committed by Facebook GitHub Bot
parent 0c3c258ce2
commit 8e29b610ae
1 changed files with 6 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
eden/scm/lib/spawn-ext/src/lib.rs
View File

@ -71,10 +71,10 @@ mod windows {
use winapi::um::winbase::CREATE_NO_WINDOW;
use winapi::um::winbase::HANDLE_FLAG_INHERIT;
// Practically MAX_HANDLE < 1000 seems to be enough.
// A larger value like 8192 adds visible overhead (ex. >5ms).
// 2048 has about 2ms overhead and seems reasonable.
const MAX_HANDLE: usize = 2048;
// At first we had 2048, however it wasn't enough in some cases
// https://fburl.com/px16lb62. We bumped it to 4096.
const MAX_HANDLE: usize = 4096;
pub fn avoid_inherit_handles(command: &mut Command) {
// Attempt to mark handles as "not inheritable".
@ -90,7 +90,9 @@ mod windows {
//
// [1]: https://github.com/processhacker/processhacker/
for handle in 1..=MAX_HANDLE {
let handle = unsafe { std::mem::transmute(handle) };
// According to https://devblogs.microsoft.com/oldnewthing/20050121-00/?p=36633
// kernel handles are always a multiple of 4
let handle = unsafe { std::mem::transmute(handle * 4) };
unsafe {
SetHandleInformation(handle, HANDLE_FLAG_INHERIT, 0)
};