166686 - nsJAR cache too frequently flushed by nsZipReaderCache::Observe()

Status: RESOLVED FIXED

(Core Graveyard :: Image: Painting, defect)

Description

[Sam Emrick]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.0.1) Gecko/20020904
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.0.1) Gecko/20020904

The neJAR cache is a good idea and for the most part works well, except the code
in nsZipReaderCache::Observe() causes some very key jar files to be dropped from
the cache and then subsequently reloaded, namely in my environment,
chrome/en-US.jar bounces in and out of cache maybe several times per url, and
also cookie.jar. The hack/fix demonstrating Observe() is the problem, simply do
RETURN at entry to Observe() and files stay cached. The intended operation of
observe real unclear to me, in my environment it fires through the
mZips.Remove() loop every time it is entered.


Reproducible: Always

Steps to Reproduce:
In nsJAR.cpp, in nsZipReaderCache::GetZip() following two lines debug code useful:
(see //SLE Debug)
...
  nsJAR* zip = NS_STATIC_CAST(nsJAR*,
NS_STATIC_CAST(nsIZipReader*,mZips.Get(&key))); // AddRefs   
  if (zip) {                                                                   
                   
    printf("nsZipReaderCache::GetZip cache hit! %s \n",path.get());  //SLE Debug
                               
#ifdef ZIP_CACHE_HIT_RATE                                                      
                   
    mZipCacheHits++;                                                           
                   
#endif                                                                         
                   
    zip->ClearReleaseTime();                                                   
                   
  }                                                                            
                   
  else {                                                                       
                   
    printf("nsZipReaderCache::GetZip cache miss! %s \n",path.get());           
                    //SLE Debug
    zip = new nsJAR();                                                         
                   

Actual Results:  
jar files are prematurely removed from cache

Expected Results:  
jar files should stay in cache until max count of cached files is reached

Comment 1

[Boris Zbarsky [:bzbarsky]]

To correct component.

It looks like Observe() evicts cache entries when we are notified that the
machine is low on RAM... Sam, what's the RAM situation like in your setup?

Comment 2

[Sam Emrick]

I've 512MB and of that 200MB free. Yea, this evening I'm trying to 
track down why it is that nsMemoryImpl::FlushMemory is being invoked. 
Maybe if somebody else or two could compile in the two printf's above 
and see if they see the same pattern on other systems and platforms. 

Comment 3

[Doug Turner (:dougt)]

I do not see any problem.  could you attach the log output you are seeing? 
Could this be on OS/2 only problem?  

Comment 4

[Sam Emrick]

Okay! This is interesting and/or wierd! Maybe it's an os/2 problem or 
moreso a vacpp problem, but not clear if/why it would be platform 
specific. Here's the scoop. 
The flush comes from nsMemoryImpl::Alloc(PRSize size)
when size=0
Problem is this - malloc() returns 0 on both error AND returns 0 when 
requested malloc size is 0. Somebody is requesting malloc zero bytes 
and this is being handled in nsMemoryImpl::Alloc as a malloc failure 
and then forces the flush.... 

Comment 5

[Sam Emrick]

It's true, verified 
nsMemoryImpl::Alloc(PRSize size)
{
    if (! size) return 0; //fix for bogus malloc falure
    ...

fixes my problem, So is this not a cross platform thing?

Comment 6

[Sam Emrick]

I propose this one line code change as a fix. 
Is there any value me doing patch file for a one liner ?

Comment 7

[Doug Turner (:dougt)]

there is the problem.  

can you fix, or at least find, whoever is calling alloc with zero?  Maybe the
simpliest solution is to protect the allocate code such that it will not fire
this memory pressure observer if the requested size was zero.

Comment 8

[Mike Kaply [:mkaply]]

Please don't change the allocator.

I want to find these bugs.

If anything, we should change the allocator to assert (maybe on OS/2)

Anyone calling malloc(0) is probably doing something they didn't mean to, so we 
should try to track that down.

Comment 9

[Sam Emrick]

Javier tells me the reason this doesn't fall into the flush path on 
windows is because malloc size=0 on windows actually does a 1 byte 
malloc and returns an address, whereas os/2 and/or vacpp for malloc 
size=0 returns 0. So what are the other platforms or compilers doing? 

Comment 10

[timeless]

it doesn't matter. as mkaply says the caller is wrong to try and allocate 0
bytes. let's fix the caller.

Comment 11

[Sam Emrick]

"finding the callers doing malloc size=0"
My spirit is willing but my tools are weak. 
I need to ask for help in doing this.

BTW, my measurements with the with my 'hack fix' would indicate, for 
os/2, eliminate the flushes is average 1.5% to 2.5% performance 
benefit, looking like benefit ranges 0% min to 5% max depending on 
testcase.

Since windows returns an address and doesn't flush, not benefit there. 
How other platforms implement malloc size=0 unknown so beneift unknown.  

Comment 12

[Daniel Veditz [:dveditz]]

If you're getting that kind of performance gain then someone's allocating 0 a
lot, so an assert should find it pretty quickly.

 nsMemoryImpl::Alloc(PRSize size)
 {
+    NS_ASSERTION(size,"allocating 0 bytes -- FIXME!!");
     void* result = MALLOC1(size);
     if (! result) {

I surfed that way a little, didn't hit the assert on windows. Maybe the alloc(0)
is os/2 specific as well?

Comment 13

[Sam Emrick]

Yes, turns out to be os2 specific, the source of the malloc(0) problem 
is gfx/src/os2/nsFontMetricsOS2.cpp, in: 

static FONTMETRICS* getMetrics( long &lFonts, PCSZ facename, HPS hps)
{
   LONG lWant = 0;
   lFonts = GFX (::GpiQueryFonts (hps, QF_PUBLIC | QF_PRIVATE,
                                  facename, &lWant, 0, 0),
                 GPI_ALTERROR);
   FONTMETRICS* pMetrics = (FONTMETRICS*)nsMemory::Alloc(lFonts * 
sizeof(FONTMETRICS));

   GFX (::GpiQueryFonts (hps, QF_PUBLIC | QF_PRIVATE, facename,
                         &lFonts, sizeof (FONTMETRICS), pMetrics),
        GPI_ALTERROR);

   return pMetrics;
}

when lFonts is sometimes set to 0, leading to an Alloc(0). 

Comment 14

[Sam Emrick]

Changed OS = OS/2 and Component = GFX

Comment 15

[Doug Turner (:dougt)]

to default.  

adding that assertion is probably a good idea.

Comment 16

[Mike Kaply [:mkaply]]

Actually, I'll take it.

First I'll post a patch for OS/2, then I'll post the memory patch

Comment 17

[Mike Kaply [:mkaply]]

Attachment: Fix

If we don't get any fonts back, bail.

Comment 18

[Mike Kaply [:mkaply]]

Attachment: Patch for memory asserts

In nsMemory and nsMemoryImpl, assert if a size of 0 is passed in.

Comment 19

[Mike Kaply [:mkaply]]

Adding dougt back, removing the install guys.

Doug, could you r= the second patch?

Comment 20

[jhp (no longer active)]

Comment on attachment 98023 [details] [diff] [review]
Fix

r=pedemont
sr=blizzard (platform specific code)

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 98023 [details] [diff] [review]
Fix

a=[e-mail] for checkin to the frozen 1.2alpha trunk.

/be

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 98025 [details] [diff] [review]
Patch for memory asserts

your only going to need this assertion in nsMemoryImpl.cpp.  Otherwise, you
will see two assertions for the same problem.  
Fix that and r=dougt

Comment 23

[Mike Kaply [:mkaply]]

First fix checked in. Waiting for 1.2

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Nominating the Fix attachment for 1.0.2

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 98025 [details] [diff] [review]
Patch for memory asserts

sr=bzbarsky if you only assert in nsMemoryImpl like dougt says

Comment 26

[Mike Kaply [:mkaply]]

Fix checked in only to nsMemoryImpl

Thinking about this, it might affect debug performance, but I have no idea how
considerably.

If we get complaints, I'll #ifdef it.