105501 - During startup nsMemory::Free(...) is being called too much !

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect, P3)

Description

[Suresh Duddi (gone)]

nsMemory::Free  52541 calls 0.02ms (without descendents)

Comment 1

[Suresh Duddi (gone)]

These calls are at startup.

Comment 2

[Suresh Duddi (gone)]

And levels of function indirection removed too.

Comment 3

[Suresh Duddi (gone)]

Attachment: Inline nsMemory::Free

Comment 4

[John Bandhauer]

Do you have stats that show that this improves speed?

Does it really inline this EXPORT'd method? On which platforms?

Do we really want to compile in the direct punt to PR_Free? This precludes being 
able to change the allocator (or do journalling allocator tricks) purely from 
within xpcom and focused on nsMemory only allocations. Maybe this does not 
really matter, but evidence that there is a gain would be appreciated.

Comment 5

[Suresh Duddi (gone)]

I am doing timing analysis on this today. Will report timing.

How would I prove that inline is happening ? Look at assembly ?

Comment 6

[Doug Turner (:dougt)]

This sounds alot like http://bugzilla.mozilla.org/show_bug.cgi?id=75620.  

Comment 7

[Suresh Duddi (gone)]

yes - this patch does inline. I looked at assembly on linux.

And my testing is showing a 4.8% improvement in startup. I am going to retest again.

In fact, folks are suggesting I should go directly to free() instead of the
indirection to PR_Free. I am going to test that out.

Comment 8

[Suresh Duddi (gone)]

4.8% was definitely too good to be true. I used the wrong build. :-(

The improvement is about 0.4% of startup. At this granularity, error in
measurement is high. The good news is it does look like an improvement. :-)

Although this is small, I vote for taking it.

Comment 9

[Suresh Duddi (gone)]

*** This bug has been marked as a duplicate of 75620 ***

Comment 10

[rpotts (gone)]

If there *really* are 52541 calls to nsMemory::Free(...) at startup, then the
'fix' is *not* to make Free(...) faster !!!  

The issue is that we are creating too many short-lived objects on the heap. 
Think about it...  Startup *should not* create lots of short-lived objects!!

So, after some Q&D hacks to nsMemory here is some data about heap utilization in
both mozilla and mfcEmbed (the embedding case)

All of this data was collected from optimized builds (starting up with
about:blank) that did not *need* to perform any component registration...

Mozilla:
========
   1. After calling SetupRegistry(...)
        Total Allocations: 469
        Live Allocations:  218
        Heap utilization:  ~38%

   2. Before calling AppShell::Run(...)
        Total Allocations: 6362
        Live Allocations:  1832
        Heap utilization:  ~29%
        NULL frees:          79  (~1.7%)

   3. After 10,000 Allocations:
        Total Allocations: 10000
        Live Allocations:   2725
        Heap utilization:   ~27%
        NULL frees:          139 (~1.9%)

   4. After 20,000 Allocations:
        Total Allocations: 20000
        Live Allocations:   4661
        Heap utilization:   ~23%
        NULL frees:          217 (~1.4%)

   5. After 40,000 Allocations:
        Total Allocations: 40000
        Live Allocations:   8170
        Heap utilization:   ~20%
        NULL frees:          641 (~2.0%)


MFCEmbed:
=========
   1. After calling SetupRegistry(...):
        Total Allocations: 546
        Live Allocations:  214
        Heap utilization:  ~39%

   2. Before calling OnNewBrowser(...):
        Total Allocations: 7359
        Live Allocations:   858
        Heap utilization:  ~12%
        NULL frees:           4 (~0%)
   3. After 10,000 Allocations:
        Total Allocations: 10000
        Live Allocations:   1397
        Heap utilization:   ~14%
        NULL frees:           35 (~0.4%)
   4. At shutdown:
        Total Allocations: 14750        NULL frees:          155
  

Comment 11

[rpotts (gone)]

From the data above, it is clear that we are creating too many short lived
objects!!  

It is interesting to note that in the embedding case, while we create many fewer
objects our overall heap utilization is almost HALF of what it is in mozilla !!!
 This seems VERY surprising and  BAD!!

It is also worth noting that in mozilla, it appears that we call
nsMemory::Free(...) about 1% of the time passing NULL !  These cases could
easily be short-circuited :-)
So, from this data, if work is being done to 'speed up' nsIMemory then PLEASE
focus on how to reduce the number of short lived objects being created during
initialization!!  This will 'speed up' nsMemory::Free(...) a whole bunch!

-- rick

Comment 12

[rpotts (gone)]

One last thing that i forgot to mention was that our usage of nsMemory::Alloc
and nsMemory::Free is *not* symetrical!!

We end up calling nsMemory::Free(...) with blocks that were *not* returned from
nsMemory::Alloc(...) !!

This does *not* cause heap corruption (only) because all of our 'allocators'
ultimately call down into malloc.  But if we ever *do* create custom (non-malloc
based) allocators then this issue MUST be addressed.

-- rick

Comment 13

[rpotts (gone)]

i'm reopening this bug to track the real issue which is that during startup we
are making too many shortlived memory allocations.

-- rick

Comment 14

[Asa Dotzler [:asa]]

0.9.6 is long gone. -> 0.9.7

Comment 15

[Brendan Eich [:brendan]]

stacks for short-lived objects?  spacetrace can find them.

/be

Comment 16

[Garrett Arch Blythe]

when you get around to using spacetrace, you'll find in the options or command
line switches the ability to exclude/include allocations that live a particular
length of time.  a lifespan under 1 second looks to be ideal for what you're after.

what you'll still be missing if you run today is the ability to sort by number
of allocations performed.  i am in the process of adding this for dp, so the
reality of reporting exactly what you need is not that far away.

Comment 17

[Suresh Duddi (gone)]

I agree with Rick. The FIX is to reduce the number of calls. spacetrace has all
data one needs to look at their module and see the number, amount and time spend
on allocations.

But this is far from being a person effort. Maybe I should file a couple of bugs
and make this one a tracking bug.

Comment 18

[Suresh Duddi (gone)]

Moving past 1.0 I dont believe we can significantly impact this before 1.0 We
are however going to reduce the number of allocation requests and hence the free
requests in our effort to reduce Vm usage and fragmentation.

Comment 19

[Vidar Haarr (not reading bugmail)]

So this bug should be titled 'Startup creates too many shortlived memory
allocations' ?

Comment 20

[Worcester12345]

Should mlk be added to keywords?

Comment 21

[Dietrich Ayala (:dietrich)]

is this bug valid anymore?

Comment 22

[Dietrich Ayala (:dietrich)]

closing - most recent substantive comments were 7 years ago. -> INCOMPLETE