Bug 95072 – [WK2] LayerTreeCoordinator should release unused UpdatedAtlases

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Balazs Kelemen]

UpdateAtlas's are allocated on demand in beginContentUpdate. At the time the number of required tiles is decreasing, we could release some of them. This should happen for example after zooming in and out.

[Balazs Kelemen]

Created attachment 160698 [details]
Patch

[Balazs Kelemen]

All of this is pretty much heuristic and also not very good for decoupling things. The basic goal of mine was to release some memory after zooming out and it has been satisfied (tested by printf's).

[Balazs Kelemen]

Created attachment 162488 [details]
Patch

[Jocelyn Turcotte]

The memory usage logic seems to add most of the complexity in the patch, is there a way to keep things a bit simpler?

Overall I think that we can always keep at least one UpdateAtlas, so would it work to only mark extra UpdateAtlases that were inactive for more than 10 frames and destroy them?

[Balazs Kelemen]

Created attachment 163138 [details]
Patch

[Balazs Kelemen]

I added a timer for this because otherwise we won't release memory if we become inactive (ui proc stop sending rendernextframe).

[Jocelyn Turcotte]

(In reply to comment #6)
> I added a timer for this because otherwise we won't release memory if we become inactive (ui proc stop sending rendernextframe).

You can gather other opinions about it, but I think that the added complexity of the timer isn't worth it.
It will only happen if the user does a zoom in/out and doesn't interact with the page after this, and in this case the memory we use will most likely not be required by the rest of the system.

PurgeBackingStores and suspend/resume should be able to deal with the cases where the memory will be needed.

[Balazs Kelemen]

(In reply to comment #7)
> (In reply to comment #6)
> > I added a timer for this because otherwise we won't release memory if we become inactive (ui proc stop sending rendernextframe).
> 
> You can gather other opinions about it, but I think that the added complexity of the timer isn't worth it.

Ok, CC'd some people :)

> It will only happen if the user does a zoom in/out and doesn't interact with the page after this, and in this case the memory we use will most likely not be required by the rest of the system.

In this version we can release the memory as soon as interaction ends for a while, no need to zoom out. Furthermore I don't think we can have any preassumption about how many memory the system needs. I think it's not nice if we keep up to 8x4 MB of memory even in background (8 atlas was the maximum I saw when tested this).

In my opinion the added complexity is minimal, but a bigger disadvantage of this change is that we need to allocate more times.

> 
> PurgeBackingStores and suspend/resume should be able to deal with the cases where the memory will be needed.

Maybe I'm wrong but I think PurgeBackingStores is sent only when the page is being destroyed. I don't see how suspend/resume comes to this, could you describe it?

[Jocelyn Turcotte]

Humm thought about it and we need your timer on desktop. What I was saying can only apply to devices have only one application visible at a time. Let me have a look at the rest.

[Jocelyn Turcotte]

Comment on attachment 163138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163138&action=review

> Source/WebKit2/ChangeLog:3
> +        [WK2] LayerTreeCoordinator should release unused UpdatedAtlas's

UpdateAtlases? I personally never saw the single quote syntax for plurals yet.

> Source/WebKit2/ChangeLog:8
> +        Release graphic buffers that are not used for a while in order to save memory.

that *haven't been* used for a while

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:644
> +void LayerTreeCoordinator::scheduleReleaseInactiveAtlasesIfNecessary()

You can remove IfNecessary from the name.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:657
> +    // Always keep one atlas.
> +    if (m_updateAtlases.size() <= 1) {
> +        m_releaseInactiveAtlasesTimer.stop();
> +        return;
> +    }

We need to make sure that the kept atlas is the one used for tiles. In this case you could end up keeping the altas used for the overlay since it has different SupportsAlpha flag.
You also need to make sure that you won't be keeping an inactive one, and then later also keep the active one, you would end up with two while only one is needed, and you would never stop your timer.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:664
> +    Vector<OwnPtr<UpdateAtlasWithInactivityCounter> > remainingAtlases;
> +    remainingAtlases.reserveInitialCapacity(m_updateAtlases.size());
> +    remainingAtlases.uncheckedAppend(m_updateAtlases[0].release());

I might be wrong but I think that in 90% of the calls to this method you won't have any atlas to remove, in which case this vector-to-vector optimization becomes a burden.
Vector::remove should be good enough if you start from the end, or you could use a Deque.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:667
> +        OwnPtr<UpdateAtlasWithInactivityCounter>& atlas = m_updateAtlases[i];

Use a pointer directly instead of a reference to the smart pointer.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.h:143
> +    class UpdateAtlasWithInactivityCounter : public UpdateAtlas {
> +    public:
> +        UpdateAtlasWithInactivityCounter(int dimension, ShareableBitmap::Flags flags)
> +            : UpdateAtlas(dimension, flags)
> +            , m_inactivityCounter(0)
> +        { }
> +        void incrementCounter() { ++m_inactivityCounter; }
> +        void resetCounter() { m_inactivityCounter = 0; }
> +        unsigned inactivityCount() const { return m_inactivityCounter; }
> +    private:
> +        unsigned m_inactivityCounter;
> +    };

This could be more elegant.
I would put the counter directly in UpdateAtlas, then:
- rename incrementCounter() to reportInactivity(int milliseconds)
- resetCounter() to resetInactivity() (or even better trigger it in beginPaintingOnAvailableBuffer())
- and inactivityCount() to isInactive() and make it handle kInactivityTolerance itself and as a number of milliseconds instead.

[Balazs Kelemen]

Created attachment 163607 [details]
Patch

[Jocelyn Turcotte]

View in context: https://bugs.webkit.org/attachment.cgi?id=163607&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:655
> +    if (m_updateAtlases.size() <= 1) {
> +        m_releaseInactiveAtlasesTimer.stop();
> +        return;
> +    }

No need to do it here if you do it at the end too.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:658
> +    bool seenActiveNormalAtlas;

I think you should initialize this.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:662
> +        bool isNormal = atlas->flags() == ShareableBitmap::NoFlags;

isNormal doesn't tell much about its purpose, something like usableForMainTiles, nonCompositedCompatible, etc. would give a bit more context to the reader.
Also please add a comment on why we need to keep this one, we need to know that this has to be changed if we change the set of flags that the non-composited layer needs to transfer its data.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:671
> +    if (!seenActiveNormalAtlas && !!atlasToKeepAnyway)

OwnPtr has a bool operator, no need to double-not it.

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.cpp:89
> +    double now = monotonicallyIncreasingTime();

Querying the system clock isn't necessary (and in this case it will be done successively for each UpdateAtlas), just pass as a parameter the repeat delay of your timer.

[Balazs Kelemen]

Created attachment 163860 [details]
Patch

[Balazs Kelemen]

(In reply to comment #13)
> Created an attachment (id=163860) [details]
> Patch

Incorporated comments. Also I realized that the best point to kick the timer is when we created a new atlas.

[Kenneth Rohde Christiansen]

Comment on attachment 163860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163860&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:655
> +    bool seenActiveNormalAtlas = false;

didFindActiveAtlas = false; what is a normal altas? really confusing for people not knowing the code

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:656
> +    for (int i = m_updateAtlases.size() - 1;  i >= 0; --i) {

size_t? unsigned?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:657
> +        UpdateAtlas* atlas = m_updateAtlases[i].get();

Do you really need the pointer here?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:660
> +        bool usableForNonCompositedContent = atlas->flags() == ShareableBitmap::NoFlags;

why so confusing? why not just bool supportsAlpha; Keep the code consistent

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.h:58
> +    void recordInactivity(double seconds)
> +    {
> +        ASSERT(!m_isUsed);
> +        m_inactiveSeconds += seconds;
> +    }
> +    bool isInactive() const
> +    {
> +        const double inactiveSecondsTolerance = 3;
> +        return !m_isUsed && m_inactiveSeconds > inactiveSecondsTolerance;
> +    }
> +    void resetInactivity() { m_inactiveSeconds = 0; }
> +    bool isUsed() const { return m_isUsed; }

Wouldn't something like lastAccessed() not make more sense?

[Jocelyn Turcotte]

Comment on attachment 163860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163860&action=review

Here's another round, sorry for being a bitch, but I think that this code has to stay simple to keep it maintainable.

> Source/WebKit2/ChangeLog:3
> +        [WK2] LayerTreeCoordinator should release unused UpdatedAtlas's

UpdateAtlases

> Source/WebKit2/ChangeLog:8
> +        Release graphic buffers that are not used for a while in order to save memory.

that *haven't been* used for a while

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:654
> +{
> +    // We always want to keep one atlas for non composited content.
> +
> +    OwnPtr<UpdateAtlas> atlasToKeepAnyway;

This comment would fit better above "if (!seenActiveNormalAtlas && !atlasToKeepAnyway && usableForNonCompositedContent)"

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:674
> +        if (m_updateAtlases.size())
> +            m_updateAtlases.first()->resetInactivity();

I might be missing something but I think that this shouldn't be needed since it will be handled by atlasToKeepAnyway on the next iteration.

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.h:47
> +    void recordInactivity(double seconds)

record is usually related to sound or disk saving, I personally find it confusing in this context.

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.h:55
> +        return !m_isUsed && m_inactiveSeconds > inactiveSecondsTolerance;

I don't understand why you need m_isUsed, why not unconditionally increment m_inactiveSeconds on every turn?
With this implementation in seems like "m_inactiveSeconds > inactiveSecondsTolerance" implies that "m_isUsed == false".

[Jocelyn Turcotte]

(In reply to comment #15)
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:660
> > +        bool usableForNonCompositedContent = atlas->flags() == ShareableBitmap::NoFlags;
> 
> why so confusing? why not just bool supportsAlpha; Keep the code consistent
> 

See previous comments, this was to give the purpose that his is related to keep only an atlas that can handle main content. A comment could be explaining this as well.

[Balazs Kelemen]

Created attachment 163872 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 163872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163872&action=review

r=me if you make those two changes. Please wait a few hours before committing in case Kenneth still has concerns.

> Source/WebKit2/ChangeLog:23
> +        (WebKit::UpdateAtlas::didSwapBuffers):

Please add a note why you removes the buildLayoutIfNeeded() call and why you think it's alright to do.

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.h:49
> +        ASSERT(!m_isUsed);

ASSERT(!isInUse())

[Kenneth Rohde Christiansen]

Comment on attachment 163872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163872&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:653
> +    bool seenActiveAtlasForNonCompositedContent = false;

seen sounds weird, why not "found"

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:660
> +            // We always want to keep one atlas for non composited content.

non-composited. Why not add this on top of "atlasToKeepAnyway" maybe call it "atlasForNonCompositedContent" making things clear

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.cpp:35
> +    , m_inactiveSeconds(0)

m_inactivityInSeconds(0)

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.h:47
> +    void reportInactivity(double seconds)

addTimeInactive()
resetTimeInactive()

[Balazs Kelemen]

Created attachment 163884 [details]
Patch

[Balazs Kelemen]

(In reply to comment #20)
> (From update of attachment 163872 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163872&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:653
> > +    bool seenActiveAtlasForNonCompositedContent = false;
> 
> seen sounds weird, why not "found"

fixed.

> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:660
> > +            // We always want to keep one atlas for non composited content.
> 
> non-composited. Why not add this on top of "atlasToKeepAnyway" maybe call it "atlasForNonCompositedContent" making things clear

No because it will not always be the atlas for non-composited content. I moved the comment because Jocelyn asked to, but I agree it's better at the start so I moved back. :)

> 
> > Source/WebKit2/WebProcess/WebPage/UpdateAtlas.cpp:35
> > +    , m_inactiveSeconds(0)
> 
> m_inactivityInSeconds(0)
> 

Ok.

> > Source/WebKit2/WebProcess/WebPage/UpdateAtlas.h:47
> > +    void reportInactivity(double seconds)
> 
> addTimeInactive()
> resetTimeInactive()

Fixed.

[Kenneth Rohde Christiansen]

Comment on attachment 163884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163884&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:656
> +        UpdateAtlas* atlas = m_updateAtlases[i].get();

I dont see why RefPtr<UpdateAtlas> atas shouldn't work? You can still do atlas->isInactive() etc

[Jocelyn Turcotte]

Comment on attachment 163884 [details]
Patch

Looks good.

[Jocelyn Turcotte]

(In reply to comment #23)
> (From update of attachment 163884 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163884&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:656
> > +        UpdateAtlas* atlas = m_updateAtlases[i].get();
> 
> I dont see why RefPtr<UpdateAtlas> atas shouldn't work? You can still do atlas->isInactive() etc

UpdateAtlas isn't refcounted, and it doesn't need shared ownership IMO.

[Kenneth Rohde Christiansen]

Comment on attachment 163884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163884&action=review

>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:656
>>> +        UpdateAtlas* atlas = m_updateAtlases[i].get();
>> 
>> I dont see why RefPtr<UpdateAtlas> atas shouldn't work? You can still do atlas->isInactive() etc
> 
> UpdateAtlas isn't refcounted, and it doesn't need shared ownership IMO.

Ah it is an OwnPtr array

[Balazs Kelemen]

Comment on attachment 163884 [details]
Patch

Clearing flags on attachment: 163884

Committed r128473: <http://trac.webkit.org/changeset/128473>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 163884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163884&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:642
> +const double ReleaseInactiveAtlasesTimerInterval = 0.5;

It’s important to have “why” comments about magic numbers like this one. What makes 0.5 seconds a good value? The future reader of this code needs to know. Also, ideally, any constants like this would be at the top of the file, rather than down between two member functions where it’s easier to overlook.

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.h:54
> +        const double inactiveSecondsTolerance = 3;

It’s important to have “why” comments about magic numbers like this one. What makes 3 seconds a good value? The future reader of this code needs to know. Also, ideally, a constant like this would not be in a header file.

[Balazs Kelemen]

(In reply to comment #29)
> (From update of attachment 163884 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163884&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:642
> > +const double ReleaseInactiveAtlasesTimerInterval = 0.5;
> 
> It’s important to have “why” comments about magic numbers like this one. What makes 0.5 seconds a good value? The future reader of this code needs to know. Also, ideally, any constants like this would be at the top of the file, rather than down between two member functions where it’s easier to overlook.
> 
> > Source/WebKit2/WebProcess/WebPage/UpdateAtlas.h:54
> > +        const double inactiveSecondsTolerance = 3;
> 
> It’s important to have “why” comments about magic numbers like this one. What makes 3 seconds a good value? The future reader of this code needs to know. Also, ideally, a constant like this would not be in a header file.

I will try to make up with some valuable comments after I see that bots are fine with the patch.

[Balazs Kelemen]

Let's track the comment fix here.

[Balazs Kelemen]

Created attachment 164078 [details]
Patch

[Balazs Kelemen]

Created attachment 164079 [details]
Patch

[Early Warning System Bot]

Comment on attachment 164079 [details]
Patch

Attachment 164079 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13835950

[Balazs Kelemen]

Created attachment 164164 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 164164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164164&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:56
> +// Our scratch buffers become unused if no user interaction happens for a while so we can release them.
> +// We want to keep the number of allocations low so we don't check this too often.
> +static const double kReleaseInactiveAtlasesTimerInterval = 0.5;

I don't see what you mean by "number of allocations" in relation by not checking this too often. AFAIK we're only checking/destroying and not allocating anything.

This is also something that has a relationship with kInactivityToleranceInSeconds, it should never be bigger than it and it might be worth noting this. An ASSERT would be even better though you would need to have then in the same file.

> Source/WebKit2/WebProcess/WebPage/UpdateAtlas.cpp:35
> +// We want to keep the number of allocations low so we consider an atlas inactive only if it
> +// was unused for a relatively long time.
> +static const double kInactivityToleranceInSeconds = 3;

Let's try to explain this a bit more scientifically, we could say something like we want to keep the atlases alive until the user has finished interacting with the page. 3 seconds sounds should be a proper fence to group successive user interactions together.

[Balazs Kelemen]

Created attachment 164553 [details]
Patch

[Noam Rosenthal]

Comment on attachment 164553 [details]
Patch

Is this patch still valid?

[Jocelyn Turcotte]

(In reply to comment #38)
> (From update of attachment 164553 [details])
> Is this patch still valid?

The last issue discussed on IRC was that kReleaseInactiveAtlasesTimerInterval had no comment explaining the / 6 and that kInactivityToleranceInSeconds made its way back to the header file.

[Balazs Kelemen]

Let's close this bug, as the actual behavioral change has been landed long ago. Jocelyn, if I remember correctly we could not really find a good compromise about how to do the refactoring (first of all I don't like the idea to put the heuristic timing stuff into UpdateAtlas), so if you don't mind I let it to you or anybody how has a clean idea.

[Jocelyn Turcotte]

Sorry about that, I think that the patch was good but there was still one or two superficial commenting issue that went back and forth.
We can reevaluate how to deal with it since the code also changed a lot in this area.