Bug 1018486 disabled a major CC optimization on opt builds

RESOLVED FIXED in Firefox 51

Status

()

Core
XPConnect
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({regression})

50 Branch
mozilla53
regression
Points:
---

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

11 months ago
http://searchfox.org/mozilla-central/rev/594937fec2e2fc45fa9308ba2fb964816631f017/js/xpconnect/src/nsXPConnect.cpp#344,349

I don't know if clang plugin now complains if I remove that #ifdef
(Assignee)

Comment 1

11 months ago
[Tracking Requested - why for this release]:
Higher cycle collection times are bad for responsiveness
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
It would be good to add a comment to the method so people can tell that it is a "load-bearing poster". https://www.youtube.com/watch?v=QRVExJZKIT8
(Assignee)

Comment 3

11 months ago
yeah, I'm adding a comment that the QI has side effects.

FWIW, the regression is 4x more GCed objects in CC graphs (median)
(Assignee)

Comment 4

11 months ago
Created attachment 8818317 [details] [diff] [review]
enable_unmark_gray.diff
Attachment #8818317 - Flags: review?(continuation)

Comment 5

11 months ago
Comment on attachment 8818317 [details] [diff] [review]
enable_unmark_gray.diff

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ +345,2 @@
>      nsCOMPtr<nsIXPConnectWrappedJSUnmarkGray> wjsug =
>        do_QueryInterface(aWrappedJS);

My guess as to why mystor did this was compiler warnings.  Do you mind making wjsug DebugOnly<>?
Attachment #8818317 - Flags: review?(continuation) → review+
(Assignee)

Comment 6

11 months ago
How would that now cause a warning when it wasn't before.
https://bugzilla.mozilla.org/show_bug.cgi?id=1279746#c10

Updated

11 months ago
status-firefox50: --- → wontfix
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox-esr45: --- → affected

Updated

11 months ago
Keywords: regression
(In reply to Olli Pettay [:smaug] from comment #3)
> yeah, I'm adding a comment that the QI has side effects.

The comment in the patch right now is "QIing to nsIXPConnectWrappedJSUnmarkGray may have side effects!" which seems more mysterious than it needs to be.  "This QI unmarks gray as a side-effect which avoids large amounts of wasted addref/release pairs." seems to be more in-line with the intent.

Comment 8

11 months ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8574c820840b
re-enable the accidentally disabled xpc_TryUnmarkWrappedGrayObject also in opt builds, r=ehsan
Backed out for static analysis bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8529b942c88344d0f62c40512007cc46878d1b89

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8574c820840b444f3c0040ac67c7f9edae24c110
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40535328&repo=mozilla-inbound

[task 2016-12-13T19:10:49.773115Z] 19:10:49     INFO -  /home/worker/workspace/build/src/sccache2/sccache /home/worker/workspace/build/src/clang/bin/clang -std=gnu99 -o derprint.o -c  -DNDEBUG -DTRIMMED=1 -DNSPR20 -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DLINUX2_1 -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -D_REENTRANT -DNSS_DISABLE_LIBPKIX -I/home/worker/workspace/build/src/security/nss/cmd/lib -I/home/worker/workspace/build/src/obj-firefox/security/nss/cmd/lib/lib_sectool -I/home/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/home/worker/workspace/build/src/obj-firefox/dist/include/private/nss -I/home/worker/workspace/build/src/obj-firefox/dist/include/nss -I/home/worker/workspace/build/src/obj-firefox/dist/include    -fPIC  -include /home/worker/workspace/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -MD -MP -MF .deps/derprint.o.pp -Qunused-arguments  -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wclass-varargs -Wloop-analysis -Werror=non-literal-null-conversion -Wstring-conversion -Wthread-safety -Wno-error=deprecated-declarations -Wno-error=array-bounds -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe  -g -Xclang -load -Xclang /home/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Os -fno-omit-frame-pointer    /home/worker/workspace/build/src/security/nss/cmd/lib/derprint.c
[task 2016-12-13T19:10:49.829195Z] 19:10:49     INFO -  In file included from /home/worker/workspace/build/src/obj-firefox/js/xpconnect/src/Unified_cpp_js_xpconnect_src1.cpp:110:
[task 2016-12-13T19:10:49.834285Z] 19:10:49     INFO -  /home/worker/workspace/build/src/js/xpconnect/src/nsXPConnect.cpp:345:5: error: Unused "kungFuDeathGrip" 'nsCOMPtr<nsIXPConnectWrappedJSUnmarkGray>' objects constructed from temporary values are prohibited
[task 2016-12-13T19:10:49.834375Z] 19:10:49     INFO -      nsCOMPtr<nsIXPConnectWrappedJSUnmarkGray> wjsug =
[task 2016-12-13T19:10:49.834405Z] 19:10:49     INFO -      ^
[task 2016-12-13T19:10:49.834501Z] 19:10:49     INFO -  /home/worker/workspace/build/src/js/xpconnect/src/nsXPConnect.cpp:346:7: note: Please switch all accesses to this value to go through 'wjsug', or explicitly pass 'wjsug' to `mozilla::Unused`
[task 2016-12-13T19:10:49.834542Z] 19:10:49     INFO -        do_QueryInterface(aWrappedJS);
[task 2016-12-13T19:10:49.834568Z] 19:10:49     INFO -        ^
[task 2016-12-13T19:10:49.834597Z] 19:10:49     INFO -  1 error generated.
Flags: needinfo?(bugs)
(Assignee)

Comment 10

11 months ago
I'll add some Unused << and push to try, since I can't figure out how to run static analysis locally on Fedora.
Flags: needinfo?(bugs)
(Assignee)

Comment 11

11 months ago
Created attachment 8818337 [details] [diff] [review]
enable_unmark_gray_with_unused.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84dae073d3b2d7a8d65c9e517f29d52137c247bc

Comment 12

11 months ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6c33db62e1
re-enable the accidentally disabled xpc_TryUnmarkWrappedGrayObject also in opt builds, r=ehsan

Comment 13

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f6c33db62e1
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 14

11 months ago
Comment on attachment 8818337 [details] [diff] [review]
enable_unmark_gray_with_unused.diff

Approval Request Comment
[Feature/Bug causing the regression]: bug 1018486
[User impact if declined]: Longer CC times
[Is this code covered by automated tests?]: This code runs all the time, and telemetry shows there was regression
[Has the fix been verified in Nightly?]: Just landed to m-c
[Needs manual test from QE? If yes, steps to reproduce]:  No need. telemetry is enough here.
[List of other uplifts needed for the feature/fix]: NA
[Is the change risky?]: Not risky. Bringing back the old behavior
[Why is the change risky/not risky?]: Bringing back the old behavior.
[String changes made/needed]: NA
Attachment #8818337 - Flags: approval-mozilla-beta?
Attachment #8818337 - Flags: approval-mozilla-aurora?
Comment on attachment 8818337 [details] [diff] [review]
enable_unmark_gray_with_unused.diff

take cycle collection regression fix for aurora52
Attachment #8818337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox53: ? → +
tracking-firefox52: ? → +

Comment 16

11 months ago
Comment on attachment 8818337 [details] [diff] [review]
enable_unmark_gray_with_unused.diff

Fix a higher cycle collection. Beta51+. Should be in 51 beta 8.
Attachment #8818337 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 17

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/35f19f6baa6c
status-firefox52: affected → fixed

Comment 18

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5aec4a29e94f
status-firefox51: affected → fixed

Comment 19

11 months ago
Track 51+ as regression.
tracking-firefox51: ? → +
Should we be considering this for esr45 as well?
Flags: needinfo?(bugs)
(Assignee)

Comment 21

11 months ago
The code in esr45 is fine
https://dxr.mozilla.org/mozilla-esr45/source/js/xpconnect/src/nsXPConnect.cpp#310
since Bug 1279746 landed to FF50.
Flags: needinfo?(bugs)
status-firefox-esr45: affected → unaffected
You need to log in before you can comment on or make changes to this bug.