Closed Bug 877036 Opened 11 years ago Closed 11 years ago

Make SharedMemory memory reporter threadsafe


(Core :: XPCOM, defect)

Not set





(Reporter: mattwoodrow, Assigned: mattwoodrow)



(Whiteboard: [MemShrink])


(2 files, 1 obsolete file)

With async video enabled, we sometimes allocate shared memory from the video decoder thread.
Attachment #755149 - Flags: review?(n.nethercote)
Whiteboard: [MemShrink]
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?
You're right, we definitely do need to do this.

I just blindly assumed the macro handled this, which doesn't make any sense.
Attachment #755149 - Attachment is obsolete: true
Attachment #755149 - Flags: review?(n.nethercote)
Attachment #755182 - Flags: review?(justin.lebar+bug)
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.
> unless you've done that already.

I mean, in one of the related bugs.
Attachment #755182 - Flags: review?(justin.lebar+bug)
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.
>   static bool registered;
>   if (!registered) {

This isn't threadsafe; it needs to do a compare-and-swap.
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?
> 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.
Attachment #755808 - Flags: review?(justin.lebar+bug)
Comment on attachment 755808 [details] [diff] [review]
Make SharedMemory's reporter threadsafe v3


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.
Attachment #755808 - Flags: review?(justin.lebar+bug) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.