Improve the dump-arrays performance on Windows #66973

hjyamauchi · 2023-06-28T00:40:49Z

Use the HeapWalk API for heap iteration instead of the
Heap32First/Next API, which was known to be slow. Since HeapWalk only
works locally, it requires using a remote thread and a DLL.

compnerd · 2023-06-28T17:58:39Z

tools/swift-inspect/Package.swift

@@ -5,6 +5,9 @@ import PackageDescription

 let package = Package(
    name: "swift-inspect",
+    products: [
+        .library(name: "SwiftInspectHeapWalk", type: .dynamic, targets: ["SwiftInspectHeapWalk"]),


I think that SwiftInspectClient might be a better name? I don't think that we should assume that it is only going to be limited to HeapWalk. We may wish to extend it further.

compnerd · 2023-06-28T17:59:07Z

tools/swift-inspect/Package.swift

@@ -16,12 +19,15 @@ let package = Package(
            dependencies: [
                "SymbolicationShims",
                .product(name: "ArgumentParser", package: "swift-argument-parser"),
+                "SwiftInspectHeapWalk",


This should be conditional on when the platform is Windows.

compnerd · 2023-06-28T17:59:22Z

tools/swift-inspect/Sources/SwiftInspectHeapWalk/SwiftInspectHeapWalk.cpp

@@ -0,0 +1,184 @@
+#if defined(_WIN32)


Missing copyright header.

tools/swift-inspect/Sources/SwiftInspectHeapWalk/SwiftInspectHeapWalk.cpp

compnerd · 2023-06-28T18:00:55Z

tools/swift-inspect/Sources/SwiftInspectHeapWalk/SwiftInspectHeapWalk.cpp

+    return 1;
+  }
+  HeapEntry *buf = (HeapEntry *)
+    MapViewOfFile(hMapFile,


We should follow LLVM style, perhaps clang-format the file?

compnerd · 2023-06-28T18:06:36Z

tools/swift-inspect/Sources/SwiftInspectHeapWalk/SwiftInspectHeapWalk.cpp

+    free(heaps);
+    heaps = (HANDLE*)malloc(heapCount * sizeof(HANDLE));


We are in C++, so prefer to use delete[] and new[].

compnerd · 2023-06-28T18:06:52Z

tools/swift-inspect/Sources/SwiftInspectHeapWalk/SwiftInspectHeapWalk.cpp

+  }
+
+  auto cleanup = [&] {
+    free(heaps);


Custom deleter with std::unique_ptr should avoid this right?

compnerd · 2023-06-28T18:08:37Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+    let sharedMemoryName = "Local\\SwiftInspectFileMapping"
+    let readEventName = "Local\\SwiftInspectReadEvent"
+    let writeEventName = "Local\\SwiftInspectWriteEvent"
+    let waitTimeoutMs = DWORD(30000)


How about extracting these constants from the C++ side into a C-only header and then using a module to share the constants across the C++ and Swift sides?

compnerd · 2023-06-28T18:09:58Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+    defer { CloseHandle(hWriteEvent) }
+
+    // The path to the dll assuming it's in the same directory as swift-inspect.
+    let dllPath: String = Bundle.main.bundlePath + "\\SwiftInspectHeapWalk.dll"


Hmm, if we use the bundle, that makes deploying this a bit more difficult as that has requirements on the layout.

compnerd · 2023-06-28T18:10:38Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+        return
+      }
+      // Allocate memory and write dllPath in the remote process
+      dllPathUtf16Len = UInt(wcslen($0) * 2)  // *2 for wide chars


I think that I would prefer using MemoryLayout rather than the hardcoded 2.

hjyamauchi · 2023-06-29T01:25:10Z

The latest revision should address all the comments.

tristanlabelle

Mind-blowing! It'd be great if the code could be broken up into a few functions so that the high-level logic can be read more easily.

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

swift-inspect will fail to build in release mode after the dump-arrays PR (swiftlang/swift#66973) due to an llvm bug (llvm/llvm-project#40056). This is a workaround for it.

hjyamauchi · 2023-07-04T18:43:46Z

@compnerd Can you take another look?

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

compnerd · 2023-07-06T05:03:11Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+    var dllPathRemote = UnsafeMutableRawPointer(bitPattern: 0)
+    var dllPathUtf16Len: UInt = 0
+    var dllPathWritten = false
+    dllPath.withCString(encodedAs: UTF16.self) {


This is unsafe. We have a path limitation to 261 characters. We absolutely need to do this inside the closure to ensure that we have a NT-style path ...

URL(fileURLWithPath: swiftInspectPath) .deletingLastPathComponent() .appendingPathComponent("SwiftInspectClient.dll") .withUnsafeFileSystemRepresentation { #"\\?\\#(String(decodingCString: $0!, as: UTF16.self))"#.withCString(encodedAs: UTF16.self) { let faAttributes: WIN32_FILE_ATTRIBUTE_DATA = .init() guard GetFileAttributesExW($0, GetFileExInfoStandard, &faAttributes), faAttributes.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == 0 else { return nil } let szLength = SIZE_T(_wcslen($0) + 1) guard let allocation = VirtualAllocEx(self.process, nil, szLength, DWORD(MEM_COMIT), DWORD(PAGE_READWRITE)) else { return nil } if !WriteProcessMemory(self.process, allocation, $0, szLength, nil) { VirtualFreeEx(self.process, allocation, 0, DWORD(MEM_RELEASE) return nil } return allocation } }

Done. Can you elaborate? I understand there's a path limitation. What's a NT-style path? And how does using a closure ensure it? Or what did the original code do wrong?

The \\?\ prefix creates a NT-style path. It is a path that bypasses the Win32 layer and goes into the kernel namespace, which allows it to bypass the 261 character limitation. The closure doesn't do that, its the interior string construction which does that. The problem is that the path must be normalised first as the normalization is something that the Win32 layer normally handles. Due to the memory handling in Foundation, we need to use a closure (withUnsafeFileSystemRepresentation). That mostly normalizes the path, and then we can create the long path.

compnerd · 2023-07-06T05:03:39Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+
+  private func unloadDllAndPathRemote(
+      dwProcessId: DWORD, hKernel32: HMODULE!,
+      dllPathRemote: UnsafeMutableRawPointer) -> Bool {


Was this formatted by swift-format?

Is there a swift-format config file that's used for the existing code? If I run it with its default config, it'd change the lines that aren't touched by this PR. Any preference?

I don't think that I had saved it ... the default sounds like a good idea though. I think that there may be a few places where it will break before the first parameter which is probably the only divergence the existing code has.

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

compnerd · 2023-07-06T05:29:11Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+                                    DWORD(FILE_MAP_ALL_ACCESS),
+                                    0,
+                                    0,
+                                    SIZE_T(bufSize));


Is this formatted via swift-format?

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

compnerd · 2023-07-06T05:31:49Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+    let hookAddr = unsafeBitCast(GetProcAddress(hKernel32, "LoadLibraryW"),
+                                 to: LPTHREAD_START_ROUTINE.self)


This seems unnecessary. It resolves to the local function LoadLibraryW, so why do the dynamic lookup?

Casting the local function pointer LoadLibraryW to UnsafeMutableRawPointer or LPTHREAD_START_ROUTINE doesn't seem to work with Swift/Builtin.swift:95: Fatal error: Can't unsafeBitCast between types of different sizes. How?

let lpfnLoadLibraryW: @convention(c) (LPCWSTR?) -> HMODULE? = LoadLibraryW _ = unsafeBitCast(lpfnLoadLibraryW, to: LPTHREAD_START_ROUTINE.self)

I imagine you can even combine the two lines.

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

CodaFi · 2023-07-07T06:18:03Z

I’m no expert in win32, but this looks pretty good. I guess my gut reaction reading all of this is that anything that touches HeapLock needs documentation about avoiding further dynamic allocations until the lock is dropped. It’s so easy to re-enter the allocator holding this thing.

compnerd · 2023-07-07T15:11:15Z

tools/swift-inspect/Sources/SwiftInspectClient/SwiftInspectClient.cpp

+  HANDLE Handle;
+  explicit ScopedHandle(HANDLE Handle) noexcept : Handle(Handle) {}
+  ~ScopedHandle() noexcept {
+    if (Handle != NULL) {


Should this be checking for INVALID_HANDLE_VALUE?

As I understand, INVALID_HANDLE_VALUE != NULL and the APIs that we use here returns NULL on failures.

Correct, the values are different. I'm worried that we might try to do a CloseHandle on INVALID_HANDLE_VALUE as that is not being checked.

tools/swift-inspect/Sources/SwiftInspectClient/SwiftInspectClient.cpp

compnerd · 2023-07-07T16:18:43Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+        .deletingLastPathComponent()
+        .appendingPathComponent("SwiftInspectClient.dll")
+        .withUnsafeFileSystemRepresentation {
+      #"\\?\\#(String(decodingCString: unsafeBitCast($0!, to: UnsafePointer<UInt8>.self), as: UTF8.self))"#


Why is the unsafeBitCast needed here?

A cast from UnsafePointer<Int8> to UnsafePointer<UInt8> needed here

Is might need to be $0.baseAddress.

That doesn't seem to work.

S:\SourceCache\swift\tools\swift-inspect\Sources\swift-inspect\WindowsRemoteProcess.swift:421:45: error: value of type 'UnsafePointer<Int8>?' has no member 'baseAddress' #"\\?\\#(String(decodingCString: $0.baseAddress, as: UTF8.self))"# ~~ ^~~~~~~~~~~

S:\SourceCache\swift\tools\swift-inspect\Sources\swift-inspect\WindowsRemoteProcess.swift:421:46: error: value of type 'UnsafePointer<Int8>' has no member 'baseAddress' #"\\?\\#(String(decodingCString: $0!.baseAddress, as: UTF8.self))"#

compnerd · 2023-07-07T16:20:38Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+
+  private func unloadDllAndPathRemote(
+      dwProcessId: DWORD, hKernel32: HMODULE!,
+      dllPathRemote: UnsafeMutableRawPointer) -> Bool {


I don't think that I had saved it ... the default sounds like a good idea though. I think that there may be a few places where it will break before the first parameter which is probably the only divergence the existing code has.

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

compnerd · 2023-07-07T16:23:16Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+
+    // Unload the dll from the remote process
+    let unloadHookAddr = unsafeBitCast(GetProcAddress(hKernel32, "FreeLibrary"),
+                                       to: LPTHREAD_START_ROUTINE.self)


Similar to above for LoadLibraryW, we can probably avoid the dynamic resolution of FreeLibrary.

let lpfnFreeLibrary: (HMODULE?) -> Bool = FreeLibrary let unloadHookAddr = unsafeBitCast(lpfnFreeLibrary, to: LPTHREAD_START_ROUTINE.self)

causes a runtime cast failure

Swift/Builtin.swift:95: Fatal error: Can't unsafeBitCast between types of different sizes Current stack trace: 0 (null) 0x00007fffb7581100 swift_stdlib_reportFatalErrorInFile + 132

I'm not sure why this one doesn't work when the LoadLibraryW one worked

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

hjyamauchi · 2023-07-10T20:42:14Z

Addressed the comments except for the embedding the dll into the executable thing. I imagine that we need to write a windows program that adds the dll as a resource into the executable and run it as part of the swift-inspect build process. How do I do that? Any examples?

compnerd

This is nearly ready! I think that the question around the invocation of FreeLibrary is something we need to think through, but the rest is looking really good!

tools/swift-inspect/Sources/SwiftInspectClient/SwiftInspectClient.cpp

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

compnerd · 2023-07-13T18:08:07Z

tools/swift-inspect/Sources/swift-inspect/WindowsRemoteProcess.swift

+    }
+
+    // Unload the dll from the remote process
+    let unloadHookAddr = unsafeBitCast(


Is this correct? This is using the address of Kernel32.FreeLibrary in the swift-inspect process but invoking it in the remote process, which is not guaranteed to be loaded at the same address right? I don't think that this is safe to do. We need to have the function be bound in the process. The simplest way to do this is to have a secondary entry point that we can invoke in the injected module that can unload itself.

I think that kernel32 is special and guaranteed to be loaded at the same address across processes.

https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-entry-point-function

Because Kernel32.dll is guaranteed to be loaded in the process address space when the entry-point function is called...

I'm not sure I read it the same way - it is that it is loaded before the entry point executes, it doesn't guarantee that the module is loaded at the same address, ASLR can relocate the base address.

Yeah, thatwas a misquote, sorry.

I'm reading that kernel32 is loaded at the same address across processes, however. For example, http://www.nynaeve.net/?p=198

Isn't that assumed and necessary when we call LoadLibraryW earlier?

Yes, that is a mistake as well. Discussed this offline, and we will use the toolhelp32 to snapshot and find the HMODULE associated with kernel32.dll in the image and use GetProcAddress to get the proper location.

hjyamauchi · 2023-07-14T00:39:33Z

PTAL

Use the HeapWalk API for heap iteration instead of the Heap32First/Next API, which was known to be slow. Since HeapWalk only works locally, it requires using a remote thread and a DLL.

hjyamauchi · 2023-07-14T23:16:47Z

Updated to get the remote LoadLibraryW/FreeLibrary addresses from the module snapshot and GetProcAddress() and no to rely on them being at the same addresses across processes.

compnerd · 2023-07-15T16:39:23Z

@swift-ci please test

Clean up some casts that would cause warnings on Windows introduced in PR swiftlang#66973. Rewrite some of the path computation for the DLL path to avoid some unnecessary conversions. Split out some of the merged guards to allow for split diagnostic emissions.

…6973 Clean up the incorrect memory binding in swift-inspect introduced in PR#66973. This fixes the incorrect memory usage to mutate the `context` parameter.

swift-inspect: correct invalid memory usage introduced in #66973

swift-inspect: clean up incorrect casts introduced in #66973

hjyamauchi requested review from ahoppen, bnbarham, rintaro, hborla, slavapestov, xedin, AnthonyLatsis, QuietMisdreavus, zoecarver, hyp, egorzhdan, artemcm, CodaFi, DougGregor and tshortli as code owners June 28, 2023 00:40

hjyamauchi force-pushed the dump-arrays branch from c4ea12d to d4aeb02 Compare June 28, 2023 00:50

QuietMisdreavus removed their request for review June 28, 2023 14:42

compnerd reviewed Jun 28, 2023

View reviewed changes

hjyamauchi force-pushed the dump-arrays branch from d4aeb02 to 159469b Compare June 29, 2023 01:24

hjyamauchi force-pushed the dump-arrays branch from 159469b to 4c090b9 Compare June 29, 2023 01:29

tristanlabelle reviewed Jun 29, 2023

View reviewed changes

hjyamauchi force-pushed the dump-arrays branch from 4c090b9 to e74ca01 Compare June 30, 2023 18:59

hjyamauchi mentioned this pull request Jul 3, 2023

Add a build workaround for the swift-inspect release build compnerd/swift-build#587

Merged

compnerd reviewed Jul 6, 2023

View reviewed changes

hjyamauchi force-pushed the dump-arrays branch from e74ca01 to a4547bb Compare July 6, 2023 23:36

compnerd reviewed Jul 7, 2023

View reviewed changes

ahoppen removed their request for review July 7, 2023 17:42

hjyamauchi force-pushed the dump-arrays branch 3 times, most recently from f776ae2 to 5b7f488 Compare July 10, 2023 20:36

tshortli removed their request for review July 10, 2023 20:45

compnerd reviewed Jul 13, 2023

View reviewed changes

hjyamauchi force-pushed the dump-arrays branch from 5b7f488 to f33b243 Compare July 14, 2023 00:24

Improve the dump-arrays performance on Windows

af890f3

Use the HeapWalk API for heap iteration instead of the Heap32First/Next API, which was known to be slow. Since HeapWalk only works locally, it requires using a remote thread and a DLL.

hjyamauchi force-pushed the dump-arrays branch from f33b243 to af890f3 Compare July 14, 2023 23:14

compnerd approved these changes Jul 15, 2023

View reviewed changes

compnerd merged commit aad2b6e into swiftlang:main Jul 16, 2023

compnerd mentioned this pull request Jul 17, 2023

swift-inspect: clean up incorrect casts introduced in #66973 #67334

Merged

compnerd added a commit that referenced this pull request Jul 18, 2023

Merge pull request #67335 from compnerd/mutant

b0d6a5a

swift-inspect: correct invalid memory usage introduced in #66973

compnerd added a commit that referenced this pull request Jul 18, 2023

Merge pull request #67334 from compnerd/66973

255beb3

swift-inspect: clean up incorrect casts introduced in #66973

		free(heaps);
		heaps = (HANDLE)malloc(heapCount sizeof(HANDLE));

		let hookAddr = unsafeBitCast(GetProcAddress(hKernel32, "LoadLibraryW"),
		to: LPTHREAD_START_ROUTINE.self)

Improve the dump-arrays performance on Windows #66973

Improve the dump-arrays performance on Windows #66973

Conversation

hjyamauchi commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjyamauchi commented Jun 29, 2023

tristanlabelle left a comment

Choose a reason for hiding this comment

hjyamauchi commented Jul 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi commented Jul 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjyamauchi commented Jul 10, 2023

compnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjyamauchi commented Jul 14, 2023

hjyamauchi commented Jul 14, 2023

compnerd commented Jul 15, 2023