Bug 99990 - sCurrentPaintTimeStamp is not initialized when FrameView::paintContents returns in the middle
Summary: sCurrentPaintTimeStamp is not initialized when FrameView::paintContents retur...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: KyungTae Kim
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2012-10-22 06:16 PDT by KyungTae Kim
Modified: 2012-11-02 23:29 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.61 KB, patch)
2012-10-22 16:14 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (1.85 KB, patch)
2012-11-01 01:40 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description KyungTae Kim 2012-10-22 06:16:58 PDT
If it is TopLevelPainter, sCurrentPaintTimeStamp is set as currentTime() in the top of FrameView::paintContents,
and is reset as 0 in the bottom of the same function.
But, when the FrameView::paintContents returns in the middle, it is not reset as 0, and the value will be not updated anymore.

That value is used for MemoryCache and there was a bug that the MemoryCache is not pruned because that value is not reset.
Comment 1 KyungTae Kim 2012-10-22 16:14:07 PDT
Created attachment 170014 [details]
Patch
Comment 2 Darin Adler 2012-10-22 18:22:05 PDT
Comment on attachment 170014 [details]
Patch

(Not your fault, but why is sCurrentPaintTimeStamp a time instead of just a boolean?)

Seems to me that a better fix is to set up isTopLevelPainter after the early returns, rather than adding these two new exit sequences.
Comment 3 KyungTae Kim 2012-10-22 18:37:33 PDT
For example, sCurrentPaintTimeStamp is referenced with currentPaintTimeStamp() in MemoryCache::::pruneLiveResourcesToSize to measure elapsedTime.

void MemoryCache::pruneLiveResourcesToSize(unsigned targetSize)
{
    double currentTime = FrameView::currentPaintTimeStamp();
...
    double elapsedTime = currentTime - current->m_lastDecodedAccessTime;
    if (elapsedTime < cMinDelayBeforeLiveDecodedPrune)
        return;
    current->destroyDecodedData();

So, this value need to be a time, 
and this value should not be initialized before the paintContents is exited.
(because it can be accessed by other functions during painting)

I considered using some constructor/destructor for initializing it automatically on exit, but It will become too complex.
Comment 4 KyungTae Kim 2012-11-01 01:40:08 PDT
Created attachment 171793 [details]
Patch
Comment 5 WebKit Review Bot 2012-11-02 23:29:48 PDT
Comment on attachment 171793 [details]
Patch

Clearing flags on attachment: 171793

Committed r133391: <http://trac.webkit.org/changeset/133391>
Comment 6 WebKit Review Bot 2012-11-02 23:29:53 PDT
All reviewed patches have been landed.  Closing bug.