1119030 - Make MOZ_CRASHes unique to prevent compiler folding

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[(Away)]

Attachment: Add __LINE__ to MOZ_CRASH

Motivated by bug 1114034 comment 9.

This patch with __LINE__ adds 147K to my opt xul.dll (didn't test PGO) and 448K to xul.pdb. That seems like an acceptable price for reducing crash dump madness.

In practice there will likely still be some collisions, but this should be a good start. I'd prefer not to add more cleverness unless we need it.

Comment 1

[Mike Hommey [:glandium]]

(In reply to David Major [:dmajor] (UTC+13) from comment #0)
> Created attachment 8545528 [details] [diff] [review]
> Add __LINE__ to MOZ_CRASH
> 
> Motivated by bug 1114034 comment 9.
> 
> This patch with __LINE__ adds 147K to my opt xul.dll (didn't test PGO)

It would be interesting to know why, because it's not obvious why changing the value of what's stored at the crashing address affects the generated code. Is it that it wasn't using immediate values before, so all of them were deduped at a single address?

Comment 2

[(Away)]

Yes - previously all MOZ_CRASHes would generate the same code, and the compiler would dedupe and replace with a jmp to a shared copy.

Comment 3

[Mike Hommey [:glandium]]

(In reply to David Major [:dmajor] (UTC+13) from comment #2)
> Yes - previously all MOZ_CRASHes would generate the same code, and the
> compiler would dedupe and replace with a jmp to a shared copy.

That sounds very much like a very MSVC specific thing. I doubt gcc/clang would do that. IOW, I wouldn't expect the libxul.so and XUL sizes to change on mac, linux and android builds, but it would be good to have numbers.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8545528 [details] [diff] [review]
Add __LINE__ to MOZ_CRASH

Review of attachment 8545528 [details] [diff] [review]:
-----------------------------------------------------------------

Would you be up for restricting this to beta and below for a release cycle or two to see how much this improves things?  I realize this is not *all* that much (~0.6% of win32 release libxul size on my machine), but it's still a significant hit.

Comment 5

[(Away)]

You mean like riding the trains? :) I'd like to get this onto Aurora 36 for the sake of bug 1114034 but I don't see a pressing need to go beyond that. We could back it out well before release if it's problematic.

Numbers for PGO and other platforms coming soon.

Comment 6

[(Away)]

(I've had this change in the back of my mind for a long time, and never wrote a patch since I too was worried about the size cost. But when my debugger said "Found 216 references" that was a pretty good motivator. The xul/mozjs/gkmedias merge must have exacerbated this in recent times.)

Comment 7

[Mike Hommey [:glandium]]

I think he means like #ifndef RELEASE_BUILD, although that doesn't include beta.

Comment 8

[(Away)]

Well this is strange. Try doesn't see a big impact on Windows -- PGO or not. Those roughly-12k differences seem a lot more reasonable for this change. I can only surmise that my local build crossed over some kind of cliff in allocation patterns.

** PGO 
win32        35,939,568   35,962,608
win64        43,989,808   43,990,320
linux32      84,907,435   84,920,982
linux64      86,812,419   86,815,338

** Non PGO
win32        33,658,608   33,670,384
win64        44,197,168   44,209,968
mac         260,276,736  260,289,056
android4.0   20,384,416   20,395,493

At this point I don't think we have anything to worry about, so here goes: https://hg.mozilla.org/integration/mozilla-inbound/rev/a17fa338bf0f

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a17fa338bf0f

Comment 11

[(Away)]

Comment on attachment 8545528 [details] [diff] [review]
Add __LINE__ to MOZ_CRASH

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: Crashes are harder to diagnose
[Describe test coverage new/current, TBPL]: A day on m-c
[Risks and why]: Very low risk, this just nudges the compiler to create better code for crashes
[String/UUID change made/needed]: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9de3db57ad7