Bug 100706 – [TexMap] Remove unused textures in the texture pool.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

Previously, we did not remove entries in the texture pool of TextureMapper.
The texture pool is destroyed when TextureMapper is destroyed. It means the texture pool consumed texture memory until its destruction.
This patch removes entries on endPainting() if the entries were unused in this frame: between startPainting() and endPainting().

[Dongseong Hwang]

Created attachment 171324 [details]
Patch

[Dongseong Hwang]

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

[Noam Rosenthal]

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

[Dongseong Hwang]

Created attachment 171326 [details]
Patch

[Dongseong Hwang]

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

[Noam Rosenthal]

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

[Noam Rosenthal]

Comment on attachment 171326 [details]
Patch

See above comments.

[Dongseong Hwang]

Created attachment 171570 [details]
Patch

[Dongseong Hwang]

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

[Noam Rosenthal]

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

[Dongseong Hwang]

Created attachment 172199 [details]
Patch

[Dongseong Hwang]

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

[WebKit Review Bot]

Comment on attachment 172199 [details]
Patch

Clearing flags on attachment: 172199

Committed r133601: <http://trac.webkit.org/changeset/133601>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

ping?

[Noam Rosenthal]

I think it's still pretty early in Korea. If we can't wait for Dongsung, feel free to roll out.

[Dongseong Hwang]

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

[Dongseong Hwang]

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