Closed Bug 877036 Opened 11 years ago Closed 11 years ago

Make SharedMemory memory reporter threadsafe

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

(Whiteboard: [MemShrink])

Attachments

(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 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.
Attachment #755808 - Flags: review?(justin.lebar+bug) → review+
Status: NEW → RESOLVED
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.

Attachment

General

Created:
Updated:
Size: