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.
Attachment #8545528 - Flags: review?(jwalden+bmo)
(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?
Yes - previously all MOZ_CRASHes would generate the same code, and the compiler would dedupe and replace with a jmp to a shared copy.
(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 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.
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.
(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.)
Attachment #8545528 - Flags: review?(jwalden+bmo) → review+
I think he means like #ifndef RELEASE_BUILD, although that doesn't include beta.
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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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
Attachment #8545528 - Flags: approval-mozilla-aurora?
Attachment #8545528 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.