Closed Bug 1397130 Opened 7 years ago Closed 7 years ago

Use signed integer for gUnusedAtomCount

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

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.
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.
Attachment #8904851 - Flags: review?(nfroyd)
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.
Attachment #8904851 - Flags: review?(nfroyd)
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?
Flags: needinfo?(nfroyd)
WFM, thank you!
Flags: needinfo?(nfroyd)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7dd4be007d13
Use signed integer for gUnusedAtomCount. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/7dd4be007d13
Status: NEW → RESOLVED
Closed: 7 years ago
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?
Flags: needinfo?(xidorn+moz)
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)
Blocks: 1389826
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: