|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
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.
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 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.
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.
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+
Mind having another look at the added comment?
WFM, thank you!
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/7dd4be007d13 Use signed integer for gUnusedAtomCount. r=froydnj
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(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?
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.)
You need to log in before you can comment on or make changes to this bug.