981315 - each gfxShmSharedReadLock uses a whole page

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Andreas Gal :gal]

We need a custom allocator here I guess to pack these onto a single page.

Comment 1

[Andreas Gal :gal]

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.

Comment 2

[Bas Schouten (:bas.schouten)]

(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

[Nicholas Nethercote [inactive]]

Why does each gfxMemorySharedReadLock use a page? sizeof(gfxMemorySharedReadLock) is 32. What am I missing?

Comment 4

[Andreas Gal :gal]

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;
    }
  }
}

Comment 5

[Bas Schouten (:bas.schouten)]

(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 :)

Comment 6

[Bas Schouten (:bas.schouten)]

Attachment: Implement an allocation for sections of Shmem

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.

Comment 7

[Andreas Gal :gal]

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!

Comment 8

[Bas Schouten (:bas.schouten)]

(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.

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Implement an allocation for sections of Shmem v2

Updated to latest m-c, carrying r+.

Comment 10

[Bas Schouten (:bas.schouten)]

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.

Comment 11

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/193df828b5a2

Comment 12

[Bas Schouten (:bas.schouten)]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/193df828b5a2
https://hg.mozilla.org/mozilla-central/rev/e30ce0237dac
https://hg.mozilla.org/mozilla-central/rev/0ab20c016504

Comment 14

[Chris Lord [:cwiiis]]

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?