Stop using ReentrantMonitor in StringBundle classes

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [overhead:3K])

Attachments

(1 attachment)

Assignee

Description

11 months ago
ReentrantMonitors are major overkill for what string bundles are doing. They're never used as monitors, and the sections where they are held are guaranteed to never be re-entrant. We should be using simple mutexes instead, since they use significantly less memory and are significantly more performant.

Updated

11 months ago
Priority: -- → P3
Comment hidden (mozreview-request)
Assignee

Updated

10 months ago
Whiteboard: [overhead:3K]

Comment 2

10 months ago
mozreview-review
Comment on attachment 8993864 [details]
Bug 1473135: Stop using ReentrantMonitor for string bundle mutexes.

https://reviewboard.mozilla.org/r/258536/#review265576

Hmm, the patch looks good, but I'm surprised to hear that Monitor is noticably heavier than Mutex.

Is there any documentation on mozilla's monitor/mutex implementations and when which one should be used?
Attachment #8993864 - Flags: review?(gandalf) → review+
Assignee

Comment 3

10 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> Hmm, the patch looks good, but I'm surprised to hear that Monitor is
> noticably heavier than Mutex.

As a rule, every monitor uses at least one mutex, in addition to a cond, which is at least as heavy as a mutex plus the wait list it has to maintain.

Re-entrant monitors are generally even worse, but ours are especially bad. They just wrap PRMonitor, which contains a mutex, two conds, an owner thread, a name pointer, and a bunch of accounting data:

https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/nsprpub/pr/include/private/primpl.h#1448-1483

They're something like 224 bytes a piece, not including any private data that libc allocates for them.

> Is there any documentation on mozilla's monitor/mutex implementations and
> when which one should be used?

Unfortunately, no. The general rule is to just always use the lightest one you can get away with.
Assignee

Comment 4

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba70b8ff0f755d5575333a71267805e0ece02f6
Bug 1473135: Stop using ReentrantMonitor for string bundle mutexes. r=gandalf

Comment 5

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ba70b8ff0f7
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.