Closed Bug 1523949 Opened Last year Closed Last year

use MOZ_THREAD_LOCAL in the deadlock detector

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(2 files, 1 obsolete file)

Fewer uses of NSPR is more better.

MOZ_THREAD_LOCAL currently assumes its invocations live in the global
namespace, which may not always be true, e.g. when declaring a static
class member whose enclosing class lives in `namespace mozilla` or
similar.  We should qualify the name lookups required to always start
from the global namespace to avoid such problems.
Attachment #9040116 - Flags: review?(erahm)
This change results in somewhat nicer code and should be slightly more
performant.
Attachment #9040117 - Flags: review?(erahm)
Attachment #9040116 - Flags: review?(erahm) → review+
Comment on attachment 9040117 [details] [diff] [review]
part 2 - switch BlockingResourceBase to MOZ_THREAD_LOCAL

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

Thanks, this is a nice cleanup!

::: xpcom/threads/BlockingResourceBase.cpp
@@ +229,5 @@
>               : 0;
>  }
>  
>  PRStatus BlockingResourceBase::InitStatics() {
> +  MOZ_RELEASE_ASSERT(sResourceAcqnChainFront.init());

Probably doesn't matter, but this code is debug only so a regular assert might be more clear.
Attachment #9040117 - Flags: review?(erahm) → review+
r?emilio for the ServoBindings.toml change.
Attachment #9040166 - Flags: review?(emilio)
Attachment #9040166 - Flags: review+
Attachment #9040117 - Attachment is obsolete: true
Comment on attachment 9040166 [details] [diff] [review]
part 2 - switch BlockingResourceBase to MOZ_THREAD_LOCAL

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

::: layout/style/ServoBindings.toml
@@ +61,5 @@
>      # structs. i.e. We may use incorrect template parameters for nested structs.
>      # https://github.com/rust-lang-nursery/rust-bindgen/issues/1429
>      "mozilla::StyleTimingFunction.*",
> +    # Only used for static members, not instance members.
> +    "mozilla::ThreadLocal.*",

So from IRC discussion this doesn't work. Does adding "mozilla::detail::ThreadLocal" to https://searchfox.org/mozilla-central/source/layout/style/ServoBindings.toml#335 work?

If so feel free to land with that change with r=me
Attachment #9040166 - Flags: review?(emilio) → review+

(Otherwise please ni? me back and I'll figure out the right incantation to fix it)

Indeed, adding it to opaque-types seems to work much better, thanks!

Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cac45126a13
part 1 - make MOZ_THREAD_LOCAL name lookups work anywhere; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/26b31ea8aeda
part 2 - switch BlockingResourceBase to MOZ_THREAD_LOCAL; r=erahm,emilio
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.