Closed
Bug 981315
Opened 11 years ago
Closed 11 years ago
each gfxShmSharedReadLock uses a whole page
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: gal, Assigned: bas.schouten)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
We need a custom allocator here I guess to pack these onto a single page.
Reporter | ||
Comment 1•11 years ago
|
||
If we have more than a very small number of these active we should block on this for 1.4. If its just a couple of them maybe we can block for 1.5.
Assignee: nobody → bas
blocking-b2g: --- → 1.4?
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Andreas Gal :gal from comment #1) > If we have more than a very small number of these active we should block on > this for 1.4. If its just a couple of them maybe we can block for 1.5. We'll have as many as we have tiles. So a 'fair' amount (say 30-ish), we should simply throw a bunch of them in a single shmem (i.e. have one per TiledContentClient probably). We already discussed this in advance and decided to not block landing on it. I'll figure something out here for 1.4, this will not be hard. We can probably get away with just allocating 2048 KB and assert a layer never gets more than 512 tiles.. (it really shouldn't.. 512 * 256 * 256 * 4 is 128MB, which is bad if it happens anyway!)
![]() |
||
Comment 3•11 years ago
|
||
Why does each gfxMemorySharedReadLock use a page? sizeof(gfxMemorySharedReadLock) is 32. What am I missing?
Reporter | ||
Comment 4•11 years ago
|
||
gfxShmSharedReadLock::gfxShmSharedReadLock(ISurfaceAllocator* aAllocator) : mAllocator(aAllocator) { MOZ_COUNT_CTOR(gfxShmSharedReadLock); if (mAllocator) { #define MOZ_ALIGN_WORD(x) (((x) + 3) & ~3) if (mAllocator->AllocUnsafeShmem(MOZ_ALIGN_WORD(sizeof(ShmReadLockInfo)), mozilla::ipc::SharedMemory::TYPE_BASIC, &mShmem)) { ShmReadLockInfo* info = GetShmReadLockInfoPtr(); info->readCount = 1; } } }
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #3) > Why does each gfxMemorySharedReadLock use a page? > sizeof(gfxMemorySharedReadLock) is 32. What am I missing? Gal means gfxShmSharedReadLock :)
Updated•11 years ago
|
Summary: each gfxMemorySharedReadLock uses a whole page → each gfxShmSharedReadLock uses a whole page
Assignee | ||
Comment 6•11 years ago
|
||
This isn't the prettiest code I've ever written. But I believe it keeps the ugliness well-contained and it seems to work well. We should perhaps look into if we should store the first available block somehow, but doing that atomically is a little tricky (I had some code for this but it looked really tricky), and since in the very worst case this will really only ever iterate over 4096 / 3 entries to find a block to allocate in I do not believe it's worth adding the complexity. When we have some more time this should be done more properly.
Attachment #8390166 -
Flags: review?(jmuizelaar)
Reporter | ||
Comment 7•11 years ago
|
||
Comment on attachment 8390166 [details] [diff] [review] Implement an allocation for sections of Shmem I recommend a follow-up bug to remove gfxSharedReadLock. The overhead of using gfxShmSharedReadLock should be 0 now. I wonder if we can simplify the logic and use a fixed size of locks. 400 tiles seems plenty. If thats not enough we can do 800 or 1600. That would reduce some of the complexity (shrinking etc). Basically for now 400 and an abort would do. For high resolution screens we want to use larger tile sizes anyway. I won't block you on this but please think about it. Lastly, it seems we could do this without atomics by using 2 pages (one in each direction) as long we only allocate/deallocate on the same side always. Quick work. I am pretty sure this will fix your crash. Thanks!
Attachment #8390166 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to Andreas Gal :gal from comment #7) > Comment on attachment 8390166 [details] [diff] [review] > Implement an allocation for sections of Shmem > > I recommend a follow-up bug to remove gfxSharedReadLock. The overhead of > using gfxShmSharedReadLock should be 0 now. > > I wonder if we can simplify the logic and use a fixed size of locks. 400 > tiles seems plenty. If thats not enough we can do 800 or 1600. That would > reduce some of the complexity (shrinking etc). Basically for now 400 and an > abort would do. For high resolution screens we want to use larger tile sizes > anyway. I won't block you on this but please think about it. We could simplify it a little by fixing at 400. I was mostly aiming to solve future problems if we decide we need to atomically share more information between compositor and content client. > Lastly, it seems we could do this without atomics by using 2 pages (one in > each direction) as long we only allocate/deallocate on the same side always. It would be very hard to make that work, as in the current situation the lock can go away on the side it was allocated on, and the last unlock can happen on the other side. It might be possible to change this, but it wouldn't be simple to, I'd discourage doing it in this phase of testing.
Assignee | ||
Comment 9•11 years ago
|
||
Updated to latest m-c, carrying r+.
Attachment #8390313 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8390166 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 8390313 [details] [diff] [review] Implement an allocation for sections of Shmem v2 Review of attachment 8390313 [details] [diff] [review]: ----------------------------------------------------------------- This has landed, I'd still like Jeff to also look at it a little bit. I might have an idea on how to add unittests.
Attachment #8390313 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/193df828b5a2
Assignee | ||
Comment 12•11 years ago
|
||
Followup fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ab20c016504 In my merge I over zealously removed the macro, which apparently on OS X and Windows is defined there anyway, but on *nix is not.
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/193df828b5a2 https://hg.mozilla.org/mozilla-central/rev/e30ce0237dac https://hg.mozilla.org/mozilla-central/rev/0ab20c016504
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•11 years ago
|
blocking-b2g: 1.4? → ---
Updated•11 years ago
|
Comment 14•11 years ago
|
||
Comment on attachment 8390313 [details] [diff] [review] Implement an allocation for sections of Shmem v2 Review of attachment 8390313 [details] [diff] [review]: ----------------------------------------------------------------- Some notes, seeing as I was looking over this. ::: gfx/layers/client/TiledContentClient.cpp @@ +314,5 @@ > { > MOZ_COUNT_CTOR(gfxShmSharedReadLock); > MOZ_ASSERT(mAllocator); > if (mAllocator) { > + if (mAllocator->AllocShmemSection(MOZ_ALIGN_WORD(sizeof(ShmReadLockInfo)), &mShmemSection)) { I think this MOZ_ALIGN_WORD is unnecessary. ::: gfx/layers/ipc/ISurfaceAllocator.cpp @@ +229,5 @@ > + } > + > + if (!aShmemSection->shmem().IsWritable()) { > + ipc::Shmem tmp; > + if (!AllocUnsafeShmem(sShmemPageSize, ipc::SharedMemory::TYPE_BASIC, &tmp)) { The unnecessary MOZ_ALIGN_WORD from before should really be here around sShmemPageSize, I'd suppose, but 4096 is certainly word-aligned. Perhaps a comment should go above sShmemPageSize noting that the number should be word-aligned? @@ +282,5 @@ > +void > +ISurfaceAllocator::FreeShmemSection(mozilla::layers::ShmemSection& aShmemSection) > +{ > + MOZ_ASSERT(aShmemSection.size() == sSupportedBlockSize); > + MOZ_ASSERT(aShmemSection.offset() < sShmemPageSize - sSupportedBlockSize); should this assert have a '- sizeof(ShmemSectionHeapHeader)' too? @@ +293,5 @@ > + MOZ_ASSERT(allocHeader->mSize == aShmemSection.size()); > + > + DebugOnly<bool> success = allocHeader->mStatus.compareExchange(STATUS_ALLOCATED, STATUS_FREED); > + // If this fails something really weird is going on. > + MOZ_ASSERT(success); Should we maybe 'if (!success) { MOZ_CRASH() }' here instead?
You need to log in
before you can comment on or make changes to this bug.
Description
•