All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED

Status

RESOLVED FIXED
16 years ago
9 years ago

People

(Reporter: emrick, Assigned: mkaply)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

16 years ago
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
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?
Assignee: law → dougt
Status: UNCONFIRMED → NEW
Component: File Handling → Networking: File
Ever confirmed: true
QA Contact: sairuh → benc
(Reporter)

Comment 2

16 years ago
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

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

Comment 4

16 years ago
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.... 

(Reporter)

Comment 5

16 years ago
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?
(Reporter)

Comment 6

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

Comment 7

16 years ago
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.
(Assignee)

Comment 8

16 years ago
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.
(Reporter)

Comment 9

16 years ago
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

16 years ago
it doesn't matter. as mkaply says the caller is wrong to try and allocate 0
bytes. let's fix the caller.
(Reporter)

Comment 11

16 years ago
"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.  
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?
Keywords: nsbeta1+
(Reporter)

Comment 13

16 years ago
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). 
(Reporter)

Updated

16 years ago
Component: Networking: File → Image: GFX
OS: All → OS/2
(Reporter)

Comment 14

16 years ago
Changed OS = OS/2 and Component = GFX

Comment 15

16 years ago
to default.  

adding that assertion is probably a good idea.
Assignee: dougt → pavlov
QA Contact: benc → tpreston
(Assignee)

Comment 16

16 years ago
Actually, I'll take it.

First I'll post a patch for OS/2, then I'll post the memory patch
Assignee: pavlov → mkaply
(Assignee)

Comment 17

16 years ago
Created attachment 98023 [details] [diff] [review]
Fix

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

Comment 18

16 years ago
Created attachment 98025 [details] [diff] [review]
Patch for memory asserts

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

Comment 19

16 years ago
Adding dougt back, removing the install guys.

Doug, could you r= the second patch?
Comment on attachment 98023 [details] [diff] [review]
Fix

r=pedemont
sr=blizzard (platform specific code)
Attachment #98023 - Flags: superreview+
Attachment #98023 - Flags: review+
Comment on attachment 98023 [details] [diff] [review]
Fix

a=brendan@mozilla.org for checkin to the frozen 1.2alpha trunk.

/be
Attachment #98023 - Flags: approval+

Comment 22

16 years ago
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
Attachment #98025 - Flags: review+
(Assignee)

Comment 23

16 years ago
First fix checked in. Waiting for 1.2
Nominating the Fix attachment for 1.0.2
Keywords: mozilla1.0.2
(Assignee)

Updated

16 years ago
Attachment #98025 - Flags: superreview?(bzbarsky)
Comment on attachment 98025 [details] [diff] [review]
Patch for memory asserts

sr=bzbarsky if you only assert in nsMemoryImpl like dougt says
Attachment #98025 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 26

16 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

9 years ago
Component: Image: Painting → Image: Painting
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.