1370609 - GetMinidumpType() can take a long time during startup

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[(no longer active)]

2.7 *seconds* in this profile (this is a cold startup)
https://perfht.ml/2rR0mP6

GetFileVersionInfoSize() loads the DLL you pass to it, and loading DLLs on Windows is the slowest thing on the planet.  We should avoid doing this on startup.

Ted, what can we do about this?

Comment 1

[(Away)]

Can we just assume that all the flags are available now that XP is gone?

Comment 2

[(not currently active) Ted Mielczarek]

> Can we just assume that all the flags are available now that XP is gone?

I think this is pretty likely, given that DbgHelp 6.2 shipped in 2003 per:
https://msdn.microsoft.com/en-us/library/ms679294(VS.85).aspx

I can't find a table anywhere that says what versions shipped with what OS releases, though.

Comment 3

[(no longer active)]

Kamil, do you happen to have a bunch of Windows VMs lying around?  It would be nice if you can take a look to see what versions of dbghelp.dll ships with Windows versions above XP, especially before any service packs and the like are installed...  Thanks!

Comment 4

[(no longer active)]

Attachment: Avoid reading the version of dbghelp.dll on startup

The fix, if we can land it.

Comment 5

[Kamil Jozwiak [:kjozwiak]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #3)
> Kamil, do you happen to have a bunch of Windows VMs lying around?  It would
> be nice if you can take a look to see what versions of dbghelp.dll ships
> with Windows versions above XP, especially before any service packs and the
> like are installed...  Thanks!

I spent most of last night/this morning re-installing the base OS's as the ones that I had were all current. Once I installed the base OS, I looked into the "C:\Windows\System32\" folder and checked the version of "dbghelp.dll" via "Right Click -> Properties -> Details". Unfortunately I only have Win 7 SP1 .iso images. I tried getting the original base Win 7 images through Microsoft but I think they've stopped providing Win 7 images as mainstream support ended on January 13, 2015.

Results:

* Win Vista x64 Ultimate (6.0.6000 Build 6000) - 6.0.6000.16386 (dbghelp.dll)
* Win Vista x86 Ultimate (6.0.6000 Build 6000) - 6.0.6000.16386 (dbghelp.dll)

* Win 7 Pro SP1 x64 (6.1.7601 Build 7601) - 6.1.7601.17514 (dbghelp.dll)
* Win 7 Pro SP1 x86 (6.1.7601 Build 7601) - 6.1.7601.17514 (dbghelp.dll)

* Win 8 x64 (6.2.9200 Build 9200) - 6.2.9200.16384 (dbghelp.dll)
* Win 8 x86 (6.2.9200 Build 9200) - 6.2.9200.16384 (dbghelp.dll)

* Win 8.1 x64 (6.3.9600 Build 9600) - 6.3.9600.17787 (dbghelp.dll)
* Win 8.1 x86 (6.3.9600 Build 9600) - 6.3.9600.17787 (dbghelp.dll)

* Win 10 Home x64 (10.0.10586 Build 10586) - 10.0.10586.0 (dbghelp.dll)
* Win 10 Home x86 (10.0.10586 Build 10586) - 10.0.10586.0 (dbghelp.dll)

Just a heads up, because there's so many different version's of ISO's out there, it's really hard to distinguish which one is the "golden copy" of the base OS. I've added the exact version of the OS that were being used to gather the above data.

Ehsan, let me know if there's anything else that I can help with.

Comment 6

[(no longer active)]

That's great info Kamil, thank you!

So our code right now is trying to detect "6.1.7600.xxxx" or newer essentially.  It seems that maps to Windows 7 SP1 version, which is corroborated by some super scientific analysis I just ran[1].  This is top notch science, so it must be true.  ;-)

This made me curious to I tracked down where this code came from and I found bug 620974 comment 12 where you noticed that your code doesn't work on Windows 7 (presumably without SP1) but then you followed up with attachment 502847 [details] [diff] [review] which is the first place where this code appears as far as I can tell.  Therefore, I think what this code is really trying to detect is Windows 7 SP1 and higher.

If this analysis is true, the patch I submitted here is wrong and should be replaced with one which uses IsWindows7SP1OrGreater() (or alternatively with GetVersionEx to detect Win7SP1 and above) to replace the current GetFileVersionInfoSize-based logic.

Ted, what do you think?

[1] https://en.wikipedia.org/wiki/Windows_7 (search the page for "February 22, 2011")

Comment 7

[(not currently active) Ted Mielczarek]

That seems fine. I think the logic here might be wrong? It doesn't help that DbgHelp.dll has its own versions, but the versions that ship with Windows just use the OS release for their versioning, so it's probably impossible to actually check the file version anyway. I think we just need to check "Windows 7 or greater", whatever is easiest.

Comment 8

[(Away)]

It might even be worth checking how an older dbghelp reacts to unknown flags. If it silently ignores them, then we could get away with passing everything unconditonally.

Comment 9

[(no longer active)]

OK, I'm a bit lost on where to go, comment 7 and 8 are suggesting vastly different things.

Windows 7 or greater means our new minimum system requirements, aka the patch that is already r+ed, right?

I don't have a way to check how an older dbghelp reacts to unknown flags here.  If we want to test comment 8, I'm probably not the right assignee.  Someone would need to figure out how to get get an older dbghelp on Windows 7 or above which is our minimum supported OS now.

Can someone please advise how I should proceed?

Comment 10

[(Away)]

I should have read your comment more carefully. This is not quite right:

> So our code right now is trying to detect "6.1.7600.xxxx" or newer
> essentially.  It seems that maps to Windows 7 SP1 version

Win7 RTM is 6.1.7600
Win7 SP1 is 6.1.7601

Meaning all of our version detections ought to be succeeding nowadays, and your patch as posted should be fine.

As an extra check, since Kamil's list didn't include Win7 RTM, I checked my base Server 2008 R2 VM (the server equivalent of Win7 RTM) and indeed we have dbghelp.dll version 6.1.7600.

If you want an extra-extra check, someone with crash dump access (Ted?) could look for a crash on OS version 6.1.7600 and check that `!vadump` produces interesting data about memory protection and flags (that is, more than just base address and region size.)

Comment 11

[(not currently active) Ted Mielczarek]

I had a Windows 7 SP1 VM laying around, and I also had a little test app that just called MinidumpWriteDump, so I changed it to pass MiniDumpWithFullMemoryInfo and ran the resulting binary in the Win7 VM and it worked fine. In light of that I don't think we should do anything more complicated than your current patch.

FTR, I'm pretty sure passing newer flags to an older DbgHelp causes MinidumpWriteDump to fail, which is why I added these checks in the first place, but since Win7 is our minimum supported version of Windows we should be fine now.

Comment 12

[(no longer active)]

(In reply to David Major [:dmajor] from comment #10)
> I should have read your comment more carefully. This is not quite right:
> 
> > So our code right now is trying to detect "6.1.7600.xxxx" or newer
> > essentially.  It seems that maps to Windows 7 SP1 version
> 
> Win7 RTM is 6.1.7600
> Win7 SP1 is 6.1.7601
> 
> Meaning all of our version detections ought to be succeeding nowadays, and
> your patch as posted should be fine.

Gah!  I suppose >= means greater than or equal...  One day I'll learn how to program.  :-)

Thanks for not blindly accepting my flawed reasoning!

> As an extra check, since Kamil's list didn't include Win7 RTM, I checked my
> base Server 2008 R2 VM (the server equivalent of Win7 RTM) and indeed we
> have dbghelp.dll version 6.1.7600.

Perfect.

> If you want an extra-extra check, someone with crash dump access (Ted?)
> could look for a crash on OS version 6.1.7600 and check that `!vadump`
> produces interesting data about memory protection and flags (that is, more
> than just base address and region size.)

I think this plus comment 10 makes me quite confident in landing the patch attached here.  I will do so in a moment.  Thank you for your help here!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4da23071bc0e
Avoid reading the version of dbghelp.dll on startup; r=ted

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4da23071bc0e