Closed Bug 1495871 Opened 2 years ago Closed 2 years ago

remove chromium atomics

Categories

(Core :: IPC, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(10 files)

For the AArch64 Windows port, I wrote some dodgy code to use the x86 code from
MSVC for chromium atomics.  As you might imagine, x86(-64) and AArch64 have
completely dissimilar memory models, so while my changes might have compiled,
they were most likely semantically incorrect.  Rather than trying to puzzle out
what the exact AArch64 instruction sequences are, and inflicting the same on
some poor reviewer, let's just blow up the need for the chromium atomics in the
first place.
Their uses are easily replaced with simpler classes.
Attachment #9013810 - Flags: review?(jld)
C++11 provides guaranteed thread-safe static initialization, so we can
use that instead of ipc's baroque Singleton class.
Attachment #9013811 - Flags: review?(rjesup)
C++11 provides guaranteed thread-safe static initialization, so we can
use that instead of ipc's baroque Singleton class.
Attachment #9013812 - Flags: review?(jld)
C++11 provides guaranteed thread-safe static initialization, so we can
use that instead of ipc's baroque Singleton class.
Attachment #9013813 - Flags: review?(choller)
C++11 provides guaranteed thread-safe static initialization, so we can
use that instead of ipc's baroque Singleton class.
Attachment #9013814 - Flags: review?(jld)
When we remove singleton.h, some webrtc code will be unhappy, because of
cargo-culted definitions from singleton.h or included headers.  Let's
fix those first
Attachment #9013815 - Flags: review?(rjesup)
C++11 provides guaranteed thread-safe static initialization, so we can
use that instead of ipc's baroque Singleton class.
Attachment #9013816 - Flags: review?(rjesup)
These headers are no longer used.

Try is telling me there may be some small additional fixes required in webrtc code.
Attachment #9013817 - Flags: review?(jld)
The only consumer of this code was Singleton, which we previously
removed, and everything that this code accomplished can be done more
simply and more foolproof-y by standard constructs these days.
Attachment #9013818 - Flags: review?(jld)
Comment on attachment 9013810 [details] [diff] [review]
remove ipc/'s copies of *AtomicSequenceNum classes

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

::: ipc/chromium/src/chrome/common/ipc_channel.cc
@@ +17,4 @@
>  namespace {
>  
>  // Global atomic used to guarantee channel IDs are unique.
> +mozilla::Atomic<int> g_last_id;

Should this be Atomic<unsigned>, given that it's printf'ed as %u?
Attachment #9013810 - Flags: review?(jld) → review+
Attachment #9013812 - Flags: review?(jld) → review+
Attachment #9013814 - Flags: review?(jld) → review+
Attachment #9013817 - Flags: review?(jld) → review+
Attachment #9013818 - Flags: review?(jld) → review+
Thanks for getting rid of this code.
Attachment #9013813 - Flags: review?(choller) → review+
Attachment #9013811 - Flags: review?(rjesup) → review+
jesup indicated in an IRC conversation that touching imported WebRTC code isn't really desirable.  In light of that, I think what we want to do is:

1) Keep the C++11 statics changes for our own code, since IMHO it's clearer.
2) Redo singleton.h using C++11 atomics, which should be relatively straightforward.

It's *possible* that step 2 will, since it removes the need for Chromium atomics, still require touching WebRTC headers.  Would that be acceptable if it happened?

Jed, WDYT about the above plan?  Are you comfortable reviewing the patch for part 2?
Flags: needinfo?(rjesup)
Flags: needinfo?(jld)
I could review that patch, but that Singleton dependency isn't from upstream WebRTC; it's something we changed locally in bug 1219339.


WebRTC has its own renamed import of the Chromium base library, so generally they shouldn't need to borrow our Chromium bits like that unless it's our change.  Also, the upstream repo doesn't have static_instance.h at all anymore — they removed it a year ago in https://webrtc.googlesource.com/src/+/779d3657ef12b65eb55455c6f415b35522c07c97 — so we might not even need to merge it the next time it's updated.
Flags: needinfo?(jld)
Comment on attachment 9013816 [details] [diff] [review]
use C++11 statics in static_instance.h

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

r+; removed from upstream so we don't have to carry this
Attachment #9013816 - Flags: review?(rjesup) → review+
Comment on attachment 9013815 [details] [diff] [review]
fix webrtc header cargo culting

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

already fixed in upstream, r+
Attachment #9013815 - Flags: review?(rjesup) → review+
Needed this to fix Windows builds.  upstream seems to have already done this:

https://webrtc.googlesource.com/src/+/master/modules/video_capture/windows/device_info_ds.h#14
Attachment #9014573 - Flags: review?(rjesup)
Attachment #9014573 - Flags: review?(rjesup) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2296a2feb192
remove ipc/'s copies of *AtomicSequenceNum classes; r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdd41eaece0f
use C++11 statics in CamerasChild; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc7db6d7326
use C++11 statics in ipc_channel_posix.cc; r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/d95dd4c9cdca
use C++11 statics for Faulty instance; r=decoder
https://hg.mozilla.org/integration/mozilla-inbound/rev/06a1c7fa3ba9
use C++11 statics in time_win.cc; r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe17ae969d6
fix webrtc header cargo culting; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/1815bfcf7984
use C++11 statics in static_instance.h; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a04672f049
fix windows includes of singleton.h; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/323979f32e2d
remove singleton headers from ipc/; r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/36356f99018a
remove chromium atomics code; r=jld
We've landed this, I guess ni? on jesup is silly now.
Flags: needinfo?(rjesup)
You need to log in before you can comment on or make changes to this bug.