877036 - Make SharedMemory memory reporter threadsafe

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Make SharedMemory's reporter threadsafe

With async video enabled, we sometimes allocate shared memory from the video decoder thread.

Comment 1

[Justin Lebar (not reading bugmail)]

Don't we need to change SharedMemory::Create and company to atomically modify the counter?  (Or we can make the counter an atomic from mfbt/Atomics.h.)  Or did you do this already?

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Make SharedMemory's reporter threadsafe v2

You're right, we definitely do need to do this.

I just blindly assumed the macro handled this, which doesn't make any sense.

Comment 3

[Justin Lebar (not reading bugmail)]

Oh, you also need to change the constructor, unless you've done that already.  There's a CAS operator you can use in Atomics.h, if you want.

Comment 4

[Justin Lebar (not reading bugmail)]

> unless you've done that already.

I mean, in one of the related bugs.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Which constructor do I need to change?

The gShmemAllocated/gShmemMapped values should be statically initialized to 0.

mAllocSize/mMappedSize are per-instance and don't need to be accessed from multiple threads.

Comment 6

[Justin Lebar (not reading bugmail)]

>   static bool registered;
>   if (!registered) {

This isn't threadsafe; it needs to do a compare-and-swap.

Comment 7

[Matt Woodrow (:mattwoodrow)]

So, Atomic<int64_t> doesn't work on our 32 bit platforms (which is most of them).

I think switching to a 32bit type on 32bit platforms should be fine here, thoughts?

Comment 8

[Justin Lebar (not reading bugmail)]

> I think switching to a 32bit type on 32bit platforms should be fine here, thoughts?

If we allocate more than 2^31 bytes of memory on a 32-bit platform we're in deep trouble, so this seems fine to me.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Make SharedMemory's reporter threadsafe v3

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 755808 [details] [diff] [review]
Make SharedMemory's reporter threadsafe v3

r=me.

The assertions are now not guaranteed to find all problems (because it's possible for two Unmapped() calls to run at the same time), but that's fine with me; you won't get a false-positive.

If you wanted to be guaranteed to catch the values going negative, you could switch to ssize_t, at the cost of one bit of space.  Or I think you can call -= such that it returns the new or old value of the variable, and you could use that to check for overflow after the fact.

Comment 11

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c14b70442b3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c14b70442b3