stylo: Use read write lock in language service

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Bug 1363172 added an RW lock.

We should use it when the language service is accessed off main thread. Bug 1362599 made it kind of work, but it doesn't handle xml:lang.
Priority: -- → P3
Summary: Use read write lock in language service → stylo: Use read write lock in language service
Assignee

Updated

2 years ago
Depends on: 1363172
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8871494 [details]
Bug 1367619 - Use RWLock when accessing font prefs service off main thread;

https://reviewboard.mozilla.org/r/142948/#review146780

::: intl/locale/nsLanguageAtomService.h:26
(Diff revision 3)
> -  nsIAtom* GetLanguageGroup(nsIAtom* aLanguage);
> +  // If aNeedsToCache is non-null, and we're not on the main thread,
> +  // this may fail and store true in aNeedsToCache, which means that
> +  // this needs to be run again with aNeedsToCache as null with the lock acquired

Can you explain more here what "needing to cache" means.  I guess it means, we need to make the nsLanguageAtomService call this again so that the result can be cached, when we're in a position (with the lock required) to do that caching.

That aNeedsToCache being null or not (at least from the comment here) seems to affect the behaviour of GetLanguageGroup is a bit tricky.

Why not just use a null return value to indicate that calling again when it's safe to cache is required, and just using a plain bool argument to indicate whether it is safe to cache?  (When it is safe to cache, we'll never return null anyway.)

Here's a suggested comment to explain what's going on, if we keep the aNeedsToCache argument.  Please adjust to fit if you change to using the null return value to indicate "couldn't cache".

// Returns the language group that the specified language is a part of.
// 
// aNeedsToCache is used for two things.  If null, it indicates that
// the nsLanguageAtomService is safe to cache the result of the
// language group lookup, either because we're on the main thread,
// or because we're on a style worker thread but the font lock has
// been acquired.  If non-null, it indicates that it's not safe to
// cache the result of the language group lookup (because we're on
// a style worker thread without the lock acquired).  In this case,
// GetLanguageGroup will store true in *aNeedsToCache true if we
// would have cached the result of a new lookup, and false if we
// were able to use an existing cached result.  Thus, callers that
// get a true *aNeedsToCache outparam value should make an effort
// to re-call GetLanguageGroup when it is safe to cache, to avoid
// recomputing the language group again later.

::: layout/style/ServoBindings.cpp:90
(Diff revision 3)
> +const nsFont*
> +ThreadSafeGetDefaultFontHelper(const nsPresContext* aPresContext, nsIAtom* aLanguage) {

static const nsFont*

::: layout/style/ServoBindings.cpp:91
(Diff revision 3)
>  static Mutex* sServoFontMetricsLock = nullptr;
> +static RWLock* sServoLangFontPrefsLock = nullptr;
> +
> +
> +const nsFont*
> +ThreadSafeGetDefaultFontHelper(const nsPresContext* aPresContext, nsIAtom* aLanguage) {

Nit: "{" on new line.

::: layout/style/ServoBindings.cpp:95
(Diff revision 3)
> +  sServoLangFontPrefsLock->ReadLock();
> +  retval = aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID,
> +                                        aLanguage, &needsCache);
> +  sServoLangFontPrefsLock->ReadUnlock();

Per the comments in RWLock.h, it's probably better to do

  {
    AutoReadLock guard(*sServoLangFontPrefsLock);
    retval = ...;
  }

and similarly below with AutoWriteLock.

::: layout/style/ServoBindings.cpp:109
(Diff revision 3)
> +bool
> +AllowedToWriteToLangFontPrefsCache()
> +{
> +#ifdef DEBUG
> +  return NS_IsMainThread() || sServoLangFontPrefsLock->OwnedByCurrentThread();
> +#else
> +  // can't verify anything in release mode.
> +  return true;
> +#endif
> +}

Please name this function mozilla::AssertIsMainThreadOrServoLangFontPrefsCacheLocked(), make it assert rather than return a boolean, so that it's similar to AssertIsMainThreadOrServoFontMetricsLocked.

(If you want to rename AssertIsMainThreadOrServoFontMetricsLocked to AssertCanAccessFontMetrics instead, and have your function be AssertCanWriteToLangFontPrefsCache, that would be fine too.)

::: xpcom/threads/RWLock.h:58
(Diff revision 3)
>  #else
>    ~RWLock();
>  #endif
>  
>  #ifdef DEBUG
> +  bool OwnedByCurrentThread();

Can you call this LockedForWritingByCurrentThread() instead?  It's not clear here that this is only something related to having acquired the write lock.
Attachment #8871494 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 years ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/adff9e8ebb3c
Use RWLock when accessing font prefs service off main thread; r=heycam
https://hg.mozilla.org/mozilla-central/rev/adff9e8ebb3c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.