[Webkit-unassigned] [Bug 30209] Cache::m_liveDecodedResource is not sorted by CachedResource::m_lastDecodedAccessTime

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30209





--- Comment #7 from Holger Freyther <zecke at selfish.org>  2009-10-19 18:06:02 PDT ---
(In reply to comment #5)
> Adam:
> 
> I disagree with r+ing this patch.
> 
> Holger said he experienced serious performance problems with earlier versions,
> and it's not clear that those problems have been resolved. He also said that
> this patch "has a problem".

The performance remark:
     - I don't have a iBench setup or any other access to a good to a
performance benchmark. So my comments on performance are hyptothetical. My
remark was that when we change the code to go through the list and insert it at
the right position we will end up with a O(n) runtime instead of the almost
constant we have now.

     - I have picked the solution that has less computation but I'm not able to
propely profile this change.


Problems:
      - Yes I will need to add a if statement to the patch and I will upload a
new one.



> Storing a painting time in FrameView was an effort to avoid excessive calls to
> currentTime(), which can be expensive. It would be nice if you could piggy-back
> on that stored value, but unfortunately it gets set to 0 when painting ends.
> Maybe we could invent something similar that you could piggy-back on.
>



> I'm worried that these new calls to currentTime() will be a performance
> problem. Did you test performance with this latest patch? If so, what were your
> results?

See the above, I'm sadly not in the position to properly benchmark it yet.



> Also, what user-visible bug are you fixing?
> I'm having a hard time
> understanding what problem arises as a result of a CachedImage being inserted
> into the LRU list with an initial m_lastDecodedAccessTime of 0. I see that
> m_liveDecodedResources is assumed to be ordered from most recently to least
> recently accessed, but I don't see a place where tying that order to
> m_lastDecodedAccessTime is important. The one thing I do see is this:
> 
>             double elapsedTime = currentTime -
> current->m_lastDecodedAccessTime;
>             if (elapsedTime < cMinDelayBeforeLiveDecodedPrune)
>                 return;
> 
> Technically, an m_lastDecodedAccessTime of 0 could cause thrash in this case,
> by allowing the cache to destroy decoded data on a resource that had just
> decoded. However, that seems almost impossible in practice, since that one
> resource would need to be big enough to bust the Cache size limits all by
> itself. Have you seen something like that happen?

Right, in the case of the m_lastDecodedAccessTime of 0 the above if statement
will almost certainly evaluate to false and the decoded data will be destroyed
but even this can only happen when the item "sank" deep enough into the queue.
So my initial idea that it will lead to other data not being freed was wrong.


I'm still in the process of trying to understand memory usage in the Qt port,
so I spent some time with the Cache trying to figure out how much is in there,
why is it kept in the cache and seein that the list is not sorted.

How would you proceed? Just add a comment that the list is not always sorted
but it is okay, do it correctly and see the performance impact?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.