Closed
Bug 1397130
Opened 7 years ago
Closed 7 years ago
Use signed integer for gUnusedAtomCount
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(1 file)
This variable can go negative at intermediate state in parallel, so it seems to be safer to have it being signed rather than unsigned. I guess it isn't the real reason of bug 1397052, but it could potentially avoid some unnecessary gc of atom table in at least some extreme case.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Given bug 1397052 comment 8, I believe this would hide the problem deeper, and we may defer the atom table GC when it goes wrong, but that's much better than continuously freezing the content.
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8904851 [details] Bug 1397130 - Use signed integer for gUnusedAtomCount. https://reviewboard.mozilla.org/r/176624/#review181994 I don't understand why this is materially better than using an unsigned integer here.
Attachment #8904851 -
Flags: review?(nfroyd)
Assignee | ||
Comment 4•7 years ago
|
||
It is better because when the count is decrement from zero, we can actually get a negative value rather than overflow to a large unsigned value. This should lead to a more reliable (signed) comparison in Atom::Release. Note that even after we fix bug 1397052, we can still hit this overflow in intermediate state during concurrency, so using signed integer should still stop us from being trapped by overflowed large integer in some cases.
Assignee | ||
Updated•7 years ago
|
Attachment #8904851 -
Flags: review?(nfroyd)
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8904851 [details] Bug 1397130 - Use signed integer for gUnusedAtomCount. https://reviewboard.mozilla.org/r/176624/#review182090 ::: xpcom/ds/nsAtomTable.cpp:70 (Diff revision 1) > } > }; > > //---------------------------------------------------------------------- > > -static Atomic<uint32_t, ReleaseAcquire> gUnusedAtomCount(0); > +static Atomic<int32_t, ReleaseAcquire> gUnusedAtomCount(0); Ah, I see from your explanation why this is useful. Can you write an explanatory comment here about how that works?
Attachment #8904851 -
Flags: review?(nfroyd) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
Mind having another look at the added comment?
Flags: needinfo?(nfroyd)
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7dd4be007d13 Use signed integer for gUnusedAtomCount. r=froydnj
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7dd4be007d13
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 12•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4) > we can still hit this overflow in intermediate state during concurrency Hm, can you clarify the scenario when this happens?
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 13•7 years ago
|
||
Assuming an atom is hold by T1, and currently there is no unused atom, so refcnt = 1, unused count = 0. T1: Release -> refcnt = 0 T2: Atomize -> refcnt = 1 T2: -- unused count -> unused count = -1 T1: ++ unused count -> unused count = 0 In this case, the unused count temporary fails beyond zero, but inside an AddRef, so it wouldn't trigger GC. If there are several more threads running in parallel, however, Release may see a negative unused count (or overflow if unsigned). Since other than stylo, there isn't many cases that lots of threads can be accessing atoms, and the main thread is frozen in stylo, this shouldn't cause problem in practice for now as atom table GC can only be done in the main thread. But given it can potentially go wrong, I'd prefer we just do it the right way other than worrying about this again in the future if we are adding more threads to run in parallel with the main thread. (Actually, is there any chance that worker threads would reference atoms? If so, they can probably trigger this easily.)
Flags: needinfo?(xidorn+moz)
You need to log in
before you can comment on or make changes to this bug.
Description
•