Summary: | [TexMap] Remove unused textures in the texture pool. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dongseong Hwang <dongseong.hwang> | ||||||||||
Component: | Layout and Rendering | Assignee: | Dongseong Hwang <dongseong.hwang> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | noam, ossy, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 101404 | ||||||||||||
Attachments: |
|
Description
Dongseong Hwang
2012-10-29 15:25:13 PDT
Created attachment 171324 [details]
Patch
I think removing BitmapTextures is well if they are not used in this single frame. However, I do not think it is the best approach. I need your opinion :) (In reply to comment #2) > I think removing BitmapTextures is well if they are not used in this single frame. However, I do not think it is the best approach. I need your opinion :) I don't think we need a full-blown LRU. How about having a timer that removes the texture after a few seconds if it wasn't used again? Something like what we do with UpdateAtlas... Created attachment 171326 [details]
Patch
(In reply to comment #3) > (In reply to comment #2) > I don't think we need a full-blown LRU. How about having a timer that removes the texture after a few seconds if it wasn't used again? Something like what we do with UpdateAtlas... Ok, good idea. I'll try :) Do you think what time threshold is proper? The threshold of UpdateAtlas is 3 seconds. const double inactiveSecondsTolerance = 3; (In reply to comment #5) > (In reply to comment #3) > > (In reply to comment #2) > > I don't think we need a full-blown LRU. How about having a timer that removes the texture after a few seconds if it wasn't used again? Something like what we do with UpdateAtlas... > > Ok, good idea. I'll try :) > > Do you think what time threshold is proper? The threshold of UpdateAtlas is 3 seconds. > const double inactiveSecondsTolerance = 3; 3 is fine. Comment on attachment 171326 [details]
Patch
See above comments.
Created attachment 171570 [details]
Patch
(In reply to comment #6) > 3 is fine. Yes, It's done. When I read your review, I wanted to remove the code recording used time of BitmapTexture. However, I can not remove the code because all BitmapTextures have one ref conut when release timer fires. We need the recent used time of BitmapTexture to determine which one can be removed. In addition, I move the constructor and destructor of TextureMapper to TextureMapper.cpp because both need concrete BitmapTexturePool definition. Comment on attachment 171570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171570&action=review Nice! See comments. > Source/WebCore/ChangeLog:14 > + Changing cache policy is not testabled in layout tests. testabled -> testable > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:36 > + inline void use() { m_usedTime = monotonicallyIncreasingTime(); } use -> markUsed > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:37 > + inline static bool compareLRU(const BitmapTexturePoolEntry& a, const BitmapTexturePoolEntry& b) inline doesn't help you when you use it as a predicate. Also, this should be called compareTimeLastUsed > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:43 > + double m_usedTime; m_timeLastUsed Created attachment 172199 [details]
Patch
(In reply to comment #10) > (From update of attachment 171570 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171570&action=review > > Nice! See comments. > Thanks for review! > testabled -> testable > use -> markUsed > m_timeLastUsed All done. Comment on attachment 172199 [details] Patch Clearing flags on attachment: 172199 Committed r133601: <http://trac.webkit.org/changeset/133601> All reviewed patches have been landed. Closing bug. (In reply to comment #13) > (From update of attachment 172199 [details]) > Clearing flags on attachment: 172199 > > Committed r133601: <http://trac.webkit.org/changeset/133601> It broke the Qt Windows build: C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\platform\graphics\texmap\TextureMapper.cpp(61) : error C2864: 'WebCore::BitmapTexturePool::s_releaseUnusedSecondsTolerance' : only static const integral data members can be initialized within a class C:\WebKitBuildSlave\szeged-windows-1\qt-windows-32bit-release\build\Source\WebCore\platform\graphics\texmap\TextureMapper.cpp(62) : error C2864: 'WebCore::BitmapTexturePool::s_releaseUnusedTexturesTimerInterval' : only static const integral data members can be initialized within a class Could you fix it, please? ping? I think it's still pretty early in Korea. If we can't wait for Dongsung, feel free to roll out. (In reply to comment #17) > I think it's still pretty early in Korea. If we can't wait for Dongsung, feel free to roll out. Sorry. I'm here, and I'll fix immediately! (In reply to comment #18) > I'm here, and I'll fix immediately! I filed Bug 101404 and posted a patch. Additionally, if you are in CA, your time - 8 is Korea time :) |