Closed Bug 1323226 Opened 7 years ago Closed 7 years ago

Bug 1018486 disabled a major CC optimization on opt builds


(Core :: XPConnect, defect)

50 Branch
Not set



Tracking Status
firefox-esr45 --- unaffected
firefox50 --- wontfix
firefox51 + fixed
firefox52 + fixed
firefox53 + fixed


(Reporter: smaug, Assigned: smaug)



(Keywords: regression)


(2 files)

[Tracking Requested - why for this release]:
Higher cycle collection times are bad for responsiveness
It would be good to add a comment to the method so people can tell that it is a "load-bearing poster".
yeah, I'm adding a comment that the QI has side effects.

FWIW, the regression is 4x more GCed objects in CC graphs (median)
Attachment #8818317 - Flags: review?(continuation)
Comment on attachment 8818317 [details] [diff] [review]

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+
How would that now cause a warning when it wasn't before.
(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.
Pushed by
re-enable the accidentally disabled xpc_TryUnmarkWrappedGrayObject also in opt builds, r=ehsan
Backed out for static analysis bustage:

Push with failures:
Failure log:

[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/ -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)
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)
Pushed by
re-enable the accidentally disabled xpc_TryUnmarkWrappedGrayObject also in opt builds, r=ehsan
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8818337 [details] [diff] [review]

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]

take cycle collection regression fix for aurora52
Attachment #8818337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8818337 [details] [diff] [review]

Fix a higher cycle collection. Beta51+. Should be in 51 beta 8.
Attachment #8818337 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Track 51+ as regression.
Should we be considering this for esr45 as well?
Flags: needinfo?(bugs)
The code in esr45 is fine
since Bug 1279746 landed to FF50.
Flags: needinfo?(bugs)
You need to log in before you can comment on or make changes to this bug.