Closed
Bug 877036
Opened 11 years ago
Closed 11 years ago
Make SharedMemory memory reporter threadsafe
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
Details
(Whiteboard: [MemShrink])
Attachments
(2 files, 1 obsolete file)
1.54 KB,
patch
|
Details | Diff | Splinter Review | |
2.96 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
With async video enabled, we sometimes allocate shared memory from the video decoder thread.
Attachment #755149 -
Flags: review?(n.nethercote)
Updated•11 years ago
|
Whiteboard: [MemShrink]
Comment 1•11 years ago
|
||
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?
Assignee | ||
Comment 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
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•11 years ago
|
||
> unless you've done that already.
I mean, in one of the related bugs.
Updated•11 years ago
|
Attachment #755182 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 5•11 years ago
|
||
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•11 years ago
|
||
> static bool registered;
> if (!registered) {
This isn't threadsafe; it needs to do a compare-and-swap.
Assignee | ||
Comment 7•11 years ago
|
||
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•11 years ago
|
||
> 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.
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #755808 -
Flags: review?(justin.lebar+bug)
Comment 10•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
Assignee: nobody → matt.woodrow
Comment 12•11 years ago
|
||
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.
Description
•