1799385 - thread-safety checks are disabled in Nightly opt builds because MOZ_PROFILING is set

Status: NEW

(Firefox Build System :: General, defect, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

OwningThread checks for non-threadsafe refcounted classes in Nightly opt builds are enabled only if MOZ_PROFILING is off (see nsDebug.h and nsISupportsImpl.h). And since Bug 1321065 in 2016, we've defaulted Nightly to MOZ_PROFILING=1. They are enabled in all debug builds, but users don't run those.

If we want to re-enable this check for Nightly (remove the MOZ_PROFILING check), we may need see if this measurably impacts CI performance tests, etc. Or use separate builds for perf tests; or have only "official" nightlies enable it (though then we'd lose the checks in CI, so I don't like this).

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1321065

:glandium, since you are the author of the regressor, bug 1321065, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

It looks like we had enable-profiling on for Nightly non-local builds even earlier than that. I'm not suggesting reverting bug 1321065, but instead enabling OwningThread in Nightly, if the performance impact can be determined to be ok

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I'd expect enabling OwningThread checks to have significant impact on some benchmarks, not all though. Profiling with such builds wouldn't be too useful. It is already hard enough to get reliable numbers with Nightly, when we run quite a bit extra code in couple of places.

OwningThread increases the object sizes (for example common DOM elements are optimized to fit into 2 cache lines in opt builds, without owningThread) and AddRef/Release are very common operations.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

We also could decide not to run OwningThread checks in Nightly; however I suspect it will occasionally catch accidental regressions. Perhaps one answer would be to enable OwningThread in Nightly either for a portion of the population (tricky), or just for a few specific official Nightly builds (say every other day, or every Saturday, or morning builds but not evening). Enabling in CI is of less benefit I think, since we run Debug builds for all the CI tests already, which has OwningThread. We could enable it for all official Nightlies (but not for CI tests); user Nightly shouldn't be used for perf tests anyways due to other bits.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1321065

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:ahochheiden, could you have a look please?

For more information, please visit auto_nag documentation.