Closed Bug 1437772 Opened 7 years ago Closed 6 years ago

0.35 - 0.58% installer size (linux32, linux64, osx-cross, windows2012-32, windows2012-64) regression on push fbd6ca22f8417eb5833e2f7c585f69252de10216 (Mon Feb 12 2018)

Categories

(Core :: Gecko Profiler, defect, P2)

Product:
Core
Component:
Gecko Profiler
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 - fixed
firefox61 - fixed
firefox62 - fixed
firefox63 + wontfix
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: igoldan, Assigned: mstange)

References

Details

(Keywords: regression)

Attachments

(1 file)

rebased backout patch for 62
6.28 KB, patch
Details | Diff | Splinter Review
We have detected a build metrics regression from push: https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=fbd6ca22f8417eb5833e2f7c585f69252de10216 As author of one of the patches included in that push, we need your help to address this regression. Regressions: 1% installer size windows2012-32 pgo 57,656,558.00 -> 57,990,707.75 0% installer size linux32 pgo 64,197,304.08 -> 64,464,600.00 0% installer size linux64 pgo 63,493,886.67 -> 63,732,961.75 0% installer size osx-cross opt 67,332,418.21 -> 67,579,900.58 0% installer size windows2012-64 pgo 61,738,288.25 -> 61,955,546.25 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=11517 On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format. To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Build_Metrics
Component: Untriaged → Gecko Profiler
Product: Firefox → Core
:mstange Changes made in bug 785440 caused >200KB increase in installer size. Please take a look over this issue, to try and reduce this size.
Flags: needinfo?(mstange)
Bug 785440 adds AUTO_PROFILER_LABEL_FAST to lots of different places. The implementation of that has tons of inlining to avoid problems with the profiler perturbing performance too much... But the upshot is that we end up with a fair amount of code added. :(
Here's a size comparison of the "XUL" library from the Mac builds from before and after this change: $ size -m before/XUL $ size -m after/XUL Segment __TEXT: 89382912 Segment __TEXT: 90836992 +1454080 Section __text: 73350208 Section __text: 74587536 +1237328 Section __stubs: 11892 Section __stubs: 11892 +0 Section __stub_helper: 19786 Section __stub_helper: 19786 +0 Section __const: 3824954 Section __const: 3824954 +0 Section __cstring: 2472348 Section __cstring: 2685148 +212800 Section __ustring: 78902 Section __ustring: 78902 +0 Section __gcc_except_tab: 106828 Section __gcc_except_tab: 106828 +0 Section __objc_methname: 33822 Section __objc_methname: 33822 +0 Section __objc_classname: 1660 Section __objc_classname: 1660 +0 Section __objc_methtype: 36027 Section __objc_methtype: 36027 +0 Section __eh_frame: 9439752 Section __eh_frame: 9441752 +2000 total 89376179 total 90828307 +1452128 Segment __DATA: 5234688 Segment __DATA: 5234688 +0 [...] [...] total 5230984 total 5230984 +0 Segment __LINKEDIT: 63348736 Segment __LINKEDIT: 63365120 +16384 total 157966336 total 159436800 +1470464 Of the total change of 1.4MB, 1.2MB are due to extra generated code and around 200KB due to extra strings. I know what to do about the strings but I don't know what to do about the code. I'm currently comparing the assembly of a few binding methods to get a breakdown of what parts are costing how much. I'm also trying to get a count of the number of methods we instrumented; I think it's somewhere between 7000 and 10000.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)
$ otool -tV after/XUL | grep __ZN7mozilla8profiler6detail12RacyFeatures18sActiveAndFeaturesE | wc -l 8857 So probably around 8800 WebIDL methods + getters + setters. Each one of those grew, on average, by 140 bytes. Here's AttrBinding::get_specified, which grew by 126 bytes: Before, 0x10ff27d - 0x10ff250 = 0x2D = 45 bytes: > __ZN7mozilla3dom11AttrBindingL13get_specifiedEP9JSContextN2JS6HandleIP8JSObjectEEPNS0_4AttrE19JSJitGetterCallArgs: // mozilla::dom::AttrBinding::get_specified(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Attr*, JSJitGetterCallArgs) > 00000000010ff250 push rbp ; Begin of unwind block (FDE at 0x4f0e6d4) > 00000000010ff251 mov rbp, rsp > 00000000010ff254 push rbx > 00000000010ff255 push rax > 00000000010ff256 mov rbx, rcx > 00000000010ff259 mov rdi, rdx > 00000000010ff25c call __ZNK7mozilla3dom4Attr9SpecifiedEv ; mozilla::dom::Attr::Specified() const > 00000000010ff261 movzx eax, al > 00000000010ff264 movabs rcx, 0xfff9000000000000 > 00000000010ff26e or rcx, rax > 00000000010ff271 mov qword [rbx], rcx > 00000000010ff274 mov al, 0x1 > 00000000010ff276 add rsp, 0x8 > 00000000010ff27a pop rbx > 00000000010ff27b pop rbp > 00000000010ff27c ret > 00000000010ff27d After, 0x11043bb - 0x1104310 = 0xAB = 171 bytes (increase of 126 bytes): > __ZN7mozilla3dom11AttrBindingL13get_specifiedEP9JSContextN2JS6HandleIP8JSObjectEEPNS0_4AttrE19JSJitGetterCallArgs: // mozilla::dom::AttrBinding::get_specified(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Attr*, JSJitGetterCallArgs) > 0000000001104310 push rbp ; Begin of unwind block (FDE at 0x5070f3c) > 0000000001104311 mov rbp, rsp > 0000000001104314 push rbx > 0000000001104315 push rax > 0000000001104316 mov rbx, rcx > 0000000001104319 lea rax, qword [__ZN7mozilla8profiler6detail12RacyFeatures18sActiveAndFeaturesE] ; __ZN7mozilla8profiler6detail12RacyFeatures18sActiveAndFeaturesE > 0000000001104320 mov eax, dword [rax] > 0000000001104322 test eax, eax > 0000000001104324 js loc_1104330 > > 0000000001104326 mov qword [rbp+var_10], 0x0 > 000000000110432e jmp loc_1104388 > > loc_1104330: > 0000000001104330 mov rax, qword [rdi+0x78] ; CODE XREF=__ZN7mozilla3dom11AttrBindingL13get_specifiedEP9JSContextN2JS6HandleIP8JSObjectEEPNS0_4AttrE19JSJitGetterCallArgs+20 > 0000000001104334 mov qword [rbp+var_10], rax > 0000000001104338 test rax, rax > 000000000110433b je loc_1104388 > > 000000000110433d mov ecx, dword [rax+0x8000] > 0000000001104343 cmp ecx, 0x3ff > 0000000001104349 ja loc_1104382 > > 000000000110434b mov ecx, dword [rax+0x8000] > 0000000001104351 shl rcx, 0x5 > 0000000001104355 lea rsi, qword [aGetAttrspecifi] ; "get Attr.specified" > 000000000110435c mov qword [rax+rcx], rsi > 0000000001104360 mov qword [rax+rcx+8], 0x0 > 0000000001104369 lea rsi, qword [rbp+var_10] > 000000000110436d mov qword [rax+rcx+0x10], rsi > 0000000001104372 mov dword [rax+rcx+0x18], 0xfe > 000000000110437a mov dword [rax+rcx+0x1c], 0x10 > > loc_1104382: > 0000000001104382 inc dword [rax+0x8000] ; CODE XREF=__ZN7mozilla3dom11AttrBindingL13get_specifiedEP9JSContextN2JS6HandleIP8JSObjectEEPNS0_4AttrE19JSJitGetterCallArgs+57 > > loc_1104388: > 0000000001104388 mov rdi, rdx ; CODE XREF=__ZN7mozilla3dom11AttrBindingL13get_specifiedEP9JSContextN2JS6HandleIP8JSObjectEEPNS0_4AttrE19JSJitGetterCallArgs+30, __ZN7mozilla3dom11AttrBindingL13get_specifiedEP9JSContextN2JS6HandleIP8JSObjectEEPNS0_4AttrE19JSJitGetterCallArgs+43 > 000000000110438b call __ZNK7mozilla3dom4Attr9SpecifiedEv ; mozilla::dom::Attr::Specified() const > 0000000001104390 movzx eax, al > 0000000001104393 movabs rcx, 0xfff9000000000000 > 000000000110439d or rcx, rax > 00000000011043a0 mov qword [rbx], rcx > 00000000011043a3 mov rax, qword [rbp+var_10] > 00000000011043a7 test rax, rax > 00000000011043aa je loc_11043b2 > > 00000000011043ac dec dword [rax+0x8000] > > loc_11043b2: > 00000000011043b2 mov al, 0x1 ; CODE XREF=__ZN7mozilla3dom11AttrBindingL13get_specifiedEP9JSContextN2JS6HandleIP8JSObjectEEPNS0_4AttrE19JSJitGetterCallArgs+154 > 00000000011043b4 add rsp, 0x8 > 00000000011043b8 pop rbx > 00000000011043b9 pop rbp > 00000000011043ba ret > 00000000011043bb The breakdown for this function is as follows: - Check if the profiler is active: +13 bytes - Remember if the profiler is inactive so that the stack pointer decrement at the end of the function can be skipped: +10 bytes - Check whether the JSContext has a PseudoStack: +13 bytes - Check whether there's space available on the PseudoStack: +14 bytes - Putting data into the ProfileEntry: +55 bytes - Increment the PseudoStack's stack pointer: +6 bytes - Conditionally decrement the PseudoStack's stack pointer: +15 bytes 13 + 10 + 13 + 14 + 55 + 6 + 15 = 126
Bug 785440 also caused these Talos regressions: == Change summary for alert #11542 (as of Mon, 12 Feb 2018 16:47:24 GMT) == Regressions: 2% tp5n main_startup_fileio windows7-32 pgo e10s stylo 71,153,840.83 -> 72,604,103.42 2% tp5n main_startup_fileio windows7-32 opt e10s stylo 68,777,990.83 -> 70,166,662.17 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11542
I think I can get us 290KB of the 1.4MB back: 90KB from code size reductions by combining two duplicated loads (stackPointer and mPseudoStack), and 200KB from string reductions, by concatenating strings at runtime instead of at compile time. These savings are measured on the uncompressed XUL library on Mac. I don't know how well they translate into installer size savings; the try pushes suggest that some of the savings even lead to installer size increases. Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0424a75d218a4592c23d377d727b9f2ca9fd04f6 + don't duplicate stackPointer load: https://treeherder.mozilla.org/#/jobs?repo=try&revision=abbdb85b7ce143eb1b2358d6d33fa27e85d9ff22 + save unnecessary mPseudoStack load: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6555d91b54cd89b6802e45464082543683f8a94a + don't concatenate strings at compile time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9f327e3313f84c8f9c47b880b87ee9e4491bcfb (needs more code changes to give the same result in the profiler)
:mstange Please let us know when you're making a fix for this, even if you're using a separate bug.
Flags: needinfo?(mstange)
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #7) > :mstange Please let us know when you're making a fix for this, even if > you're using a separate bug. I should have said "when you're pushing the fix".
Will this make 60 cutoff?
Don't think so. We can back out the last patch in bug 785440 from Beta once we've merged; that will undo the regression.
Flags: needinfo?(mstange)
Depends on: 1443830
[Tracking Requested - why for this release]: Make sure it doesn't get lost.
tracking-firefox60: --- → ?
So we have a few options here in addition to just decreasing the code size: 1) We could enable this only in nightly-or-early-beta. 2) We could enable this only for certain interfaces, based on whitelists or blacklists or something. It would be good to understand the use cases we have here so we can figure out whether any of that makes sense....
Flags: needinfo?(mstange)
Flags: needinfo?(ehsan)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12) > So we have a few options here in addition to just decreasing the code size: > > 1) We could enable this only in nightly-or-early-beta. I'm happy to #ifdef NIGHTLY_BUILD this until this affects a web developer facing feature. More specifically, I think we can make a better case for shipping this in release builds once perf.html has a proper web developer centric view, and once the integration of perf.html into the Firefox devtools is further along. > 2) We could enable this only for certain interfaces, based on whitelists or > blacklists or something. This is worth revisiting once we've landed some improvements to the code size. I've had some more ideas for how to reduce code size since my last update to this bug. Ordered from "obvious" to "desperate": - Fold the "is profiler running" check into the "is the PseudoStack on the JSContext non-null" check by having a new field that points to the PseudoStack only if the profiler is running and is null otherwise - Don't write to the line number field, it's useless. (Would need to make sure we don't access that field for WebIDL PseudoStack frames) - Handle PseudoStack stack size overflow using a guard page instead of manual checks - Have a side-table that lists all WebIDL APIs so that we only need to push an index, instead of two strings and a category
Flags: needinfo?(mstange)
What advantage would this add to Nightly given that Nightly builds already have --enable-profiling? Is it just to make sure the code doesn't bitrot?
It does not require symbolication, it's present when stackwalking fails (e.g. through JIT code), and it can be treated specially by perf.html without having to make perf.html pattern-match C++ function names.
Boris, I'm tracking 1443830 for 60.
tracking-firefox60: ? → -
[Tracking Requested - why for this release]: see comment 11
status-firefox61: --- → affected
tracking-firefox61: --- → ?
Comment 13 sounds great to me.
Flags: needinfo?(ehsan)
Hi Markus, any recent updates on this bug? :)
Flags: needinfo?(mstange)
Priority: -- → P2
No update yet. I have some patches locally but they all depend on a bigger reworking that I haven't put up for review yet. We should do the same partial backout that we did for 60, for 61.
Flags: needinfo?(mstange)
Markus, do you plan to do a similar backout for 62? We will be building beta 1 in a couple of weeks.
Flags: needinfo?(mstange)
Flags: needinfo?(mstange)
Is this still affecting 62? Do you plan to do a backout on beta? I may have already asked on irc but if so, I lost that thread. Does this still affect 63? Thanks!
status-firefox63: --- → affected
Flags: needinfo?(mstange)
Emailing Markus to follow up.
Sorry, yes, this still affects 62, and we need another backout. Hopefully for the last time. I was hoping to get a proper fix in time but that didn't work out. Sorry I didn't respond sooner.
Flags: needinfo?(mstange)
Markus, any luck here? We still have 3 weeks till the first merge/RC builds. But I'd like to get this change into 62 as soon as possible.
Flags: needinfo?(mstange)
Ryan, can you do the same partial backout again on 62 that you did the previous times?
Flags: needinfo?(mstange) → needinfo?(ryanvm)
The backout patch doesn't apply cleanly. It'll need rebasing.
Flags: needinfo?(ryanvm) → needinfo?(mstange)
Here's the rebased patch.
Flags: needinfo?(mstange) → needinfo?(ryanvm)
(In reply to Markus Stange [:mstange] from comment #25) > Sorry, yes, this still affects 62, and we need another backout. Hopefully > for the last time. Markus, does it still affect 63? Thanks
Flags: needinfo?(mstange)
Marking as wontfix for 63 as we have our last beta today.
status-firefox63: affected → wontfix
status-firefox64: --- → affected
Depends on: 1499507
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #7) > :mstange Please let us know when you're making a fix for this, even if > you're using a separate bug. Bug 1499507 just landed which should help a bit. Can you check whether it made a difference?
Flags: needinfo?(mstange) → needinfo?(igoldan)
(In reply to Markus Stange [:mstange] from comment #32) > (In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #7) > > :mstange Please let us know when you're making a fix for this, even if > > you're using a separate bug. > > Bug 1499507 just landed which should help a bit. Can you check whether it > made a difference? Yes, it did made a difference. I'd say it fixed Windows 10 and OS X, Windows 7 partially, Linux 64 by a very small amount.
Flags: needinfo?(igoldan)
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #33) > (In reply to Markus Stange [:mstange] from comment #32) > > (In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #7) > > > :mstange Please let us know when you're making a fix for this, even if > > > you're using a separate bug. > > > > Bug 1499507 just landed which should help a bit. Can you check whether it > > made a difference? > > Yes, it did made a difference. I'd say it fixed Windows 10 and OS X, Windows > 7 partially, Linux 64 by a very small amount. That's probably good enough.
Thanks for checking! I'll mark this bug as resolved then. I don't think it's worth uplifting these patches.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64: affected → wontfix
status-firefox65: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: