120299 - Drop feedback is narrow first time

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Jan Varga [:janv]]

Comment 1

[Jan Varga [:janv]]

Attachment: possible fix

Comment 2

[Jan Varga [:janv]]

Outliner's image cache uses as key style contexts. It depends on prefilled array
of properties. So it's possible to have different entries in cache for same
image requests. I measured it, folder pane has about 50 entries in the cache.
Default/initial cache size is 64 entries.
I know image lib has own cache, but I think we shouldn't keep redundant image
requests in outliner's cache.
In that patch I chanched it to use image URL (string key) and initial size to 16
entries. It reduced number of entries to about 13 for folder pane.

Drop feedback is narrow first time, because scratch array contains new property
(drop/dropBefore/dropAfter) and thus it doesn't find image request in its cache.

Sounds reasonable ?

Comment 3

[David :Bienvenu]

I suggested doing this a while ago, and hyatt said "I'm fairly convinced that
using URIs as keys would be slower than the way it works now"

Comment 4

[Jan Varga [:janv]]

Yeah, it would be slightly slower and more malloc happy.
Ccing brendan for possible hashing tips.

Comment 5

[Brendan Eich [:brendan]]

Measure first and see if the difference isn't in the noise -- yeah, nsStringKey
is malloc-happy, but I wonder with the low call counts why this'll hurt.

If there's no opaque, unique-per-image pointer to use, it seems to me the URI
must be the key.

/be

Comment 6

[Jan Varga [:janv]]

Attachment: working patch

sigh, hand-modified patches

Comment 7

[Simon Fraser [no longer active]]

Comment on attachment 67502 [details] [diff] [review]
working patch

r=sfraser
This patch fixes the Mac drawing issues.

Comment 8

[David Hyatt]

I'm uncomfortable with this patch, since the current cache prevents 
mallocs of URIs in the common cases (straight display and scrolling).  
Also, imglib is *already* hashing in a second tier cache by URI.  

I think the real problem here is just that there's an invalidate happening 
during cell painting when an image is already available in the cache.  In 
this case, no extra invalidate needs to occur.

I would suggest adding a guard against invalidation in the code that gets 
the image.  Set a stack-based variable to true right before the call to il->
LoadImage, and then don't let the invalidate happen in the case where 
imglib responds synchronously.  

e.g.,

mImageGuard = PR_TRUE;
il->LoadImage();
mImageGuard = PR_FALSE;

and in the listener...

if (!mImageGuard)
  Invalidate();

But hashing on the URI is redundant and malloc-happy (and you'll 
repeatedly re-malloc and build the same URI over and over again).

Comment 9

[Jan Varga [:janv]]

Attachment: new patch

Comment 10

[David Hyatt]

Yeah, looks good.  Assuming it fixes the bug, sr=hyatt.

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 67514 [details] [diff] [review]
new patch

r=sfraser, this fixes the bug (so adding hyatt's sr)

Comment 12

[Jan Varga [:janv]]

Attachment: real fix

Try to get an image after load (in a case it was already cached).
Added comments.

Comment 13

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.8

Comment 14

[Jan Varga [:janv]]

landed on the trunk and branch

Comment 15

[Asa Dotzler [:asa]]

this is probably covering or the same as bug 121187

Comment 16

[Jan Varga [:janv]]

no, it is not the same