Closed Bug 1155864 Opened 6 years ago Closed 6 years ago

Rip out Windows intrinsics from Atomics.h and allow Atomic to hold 64-bit types everywhere

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file)

Summary says it all. There's more discussion in bug 1155809.
Here's the patch. Shouldn't be too controversial assuming this looks good on try.
Attachment #8594176 - Flags: review?(jwalden+bmo)
Comment on attachment 8594176 [details] [diff] [review]
Rip out Windows intrinsics from Atomics.h and allow Atomic to hold 64-bit types everywhere

Review of attachment 8594176 [details] [diff] [review]:
-----------------------------------------------------------------

I'd be interested to know what gcc generates for a 64-bit variable on a 32-bit system for this.  Some sort of global lock in libstdc++, I guess?
Attachment #8594176 - Flags: review?(jwalden+bmo) → review+
Blocks: 1154974
Wait, so should we just remove mozilla::Atomic then? Why shouldn't we just make everything std::atomic?
I do remember some quality-of-implementation concerns with Windows's <atomic> implementation -- that with some compilers it wouldn't make Relaxed operations fast enough or something.  Maybe our MSVC requirements have outstripped that old issue.

The other consideration is that Atomic<T> puts the ordering in the type, so you need only look one place to understand things.  There's a small tradeoff in that this precludes some optimizations permitted by mismatched orderings, in rare cases.  (The canonical being reference-counting implementations, I think.)  But such are rare, and arguably best not permitted.

I wouldn't mind switching to std::atomic, but I'm not sure if we're there yet.  Poking jcranmer for his thoughts on all this...
Flags: needinfo?(Pidgeot18)
FWIW, I'd vastly prefer to just use std::atomic (though I do like having the ordering in the type), but IMO we should land the patch in this bug first and see how things go before we commit ourselves.
Yeah, I agree with landing this now and doing other stuff after, if desired.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> I do remember some quality-of-implementation concerns with Windows's
> <atomic> implementation -- that with some compilers it wouldn't make Relaxed
> operations fast enough or something.  Maybe our MSVC requirements have
> outstripped that old issue.

Yeah, MSVC 2012 did <atomic> wrong.  So we have no concerns there, but...

> I wouldn't mind switching to std::atomic, but I'm not sure if we're there
> yet.  Poking jcranmer for his thoughts on all this...

std::atomic is not available on Android/B2G.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> I do remember some quality-of-implementation concerns with Windows's
> <atomic> implementation -- that with some compilers it wouldn't make Relaxed
> operations fast enough or something.  Maybe our MSVC requirements have
> outstripped that old issue.

I know there's also a few MSVC C++11 things that don't work on Windows XP. Definitely CondVar, and potentially some lesser std::atomic stuff. Off the top of my head, I'm not sure.

> I wouldn't mind switching to std::atomic, but I'm not sure if we're there
> yet.  Poking jcranmer for his thoughts on all this...

The libstdc++ support for std::atomic has been fairly abysmal, judging by the constant ratcheting of the min version we supported there. I'd hope the BSDs have moved to libc++ so that we don't actually need libstdc++-4.2 support anymore, but it's not guaranteed. In addition, there's issues with libstdcxxcompat (particularly if we support std::atomic<uint64_t> on platforms that don't have atomic uint64_t operations). And, as mentioned by nfroyd, stlport doesn't support std::atomic.

One advantage of mozilla::Atomic is that it fixes the ordering issues in a guaranteed manner.
Flags: needinfo?(Pidgeot18)
(In reply to Joshua Cranmer [:jcranmer] from comment #9)
> I know there's also a few MSVC C++11 things that don't work on Windows XP.
> Definitely CondVar, and potentially some lesser std::atomic stuff.

Do you have links for any of this? I can't make Google show me much.
Try looks green.
https://hg.mozilla.org/mozilla-central/rev/8e8f3e6e0739
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.