Closed Bug 981315 Opened 6 years ago Closed 6 years ago

each gfxShmSharedReadLock uses a whole page

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: gal, Assigned: bas.schouten)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We need a custom allocator here I guess to pack these onto a single page.
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?
(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!)
Why does each gfxMemorySharedReadLock use a page? sizeof(gfxMemorySharedReadLock) is 32. What am I missing?
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;
    }
  }
}
(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 :)
Summary: each gfxMemorySharedReadLock uses a whole page → each gfxShmSharedReadLock uses a whole page
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)
Blocks: 982128
Blocks: 983020
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+
(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.
Updated to latest m-c, carrying r+.
Attachment #8390313 - Flags: review+
Attachment #8390166 - Attachment is obsolete: true
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)
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.
blocking-b2g: 1.4? → ---
Blocks: 983157
No longer blocks: 983157
Depends on: 983157
No longer depends on: 983157
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.