Summary: | [WK2] LayerTreeCoordinator should release unused UpdatedAtlases | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Balazs Kelemen <kbalazs> | ||||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Balazs Kelemen <kbalazs> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | hausmann, jturcotte, noam | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Attachments: |
|
Description
Balazs Kelemen
2012-08-27 04:38:18 PDT
Created attachment 160698 [details]
Patch
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). Created attachment 162488 [details]
Patch
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? Created attachment 163138 [details]
Patch
I added a timer for this because otherwise we won't release memory if we become inactive (ui proc stop sending rendernextframe). (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. (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? 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. 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. Created attachment 163607 [details]
Patch
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. Created attachment 163860 [details]
Patch
(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. 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? 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". (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. Created attachment 163872 [details]
Patch
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()) 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() Created attachment 163884 [details]
Patch
(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. 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 Comment on attachment 163884 [details]
Patch
Looks good.
(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. 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 Comment on attachment 163884 [details] Patch Clearing flags on attachment: 163884 Committed r128473: <http://trac.webkit.org/changeset/128473> All reviewed patches have been landed. Closing bug. 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. (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. Let's track the comment fix here. Created attachment 164078 [details]
Patch
Created attachment 164079 [details]
Patch
Comment on attachment 164079 [details] Patch Attachment 164079 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13835950 Created attachment 164164 [details]
Patch
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. Created attachment 164553 [details]
Patch
Comment on attachment 164553 [details]
Patch
Is this patch still valid?
(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. 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. 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. |