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

RESOLVED INCOMPLETE

Status

()

defect
P3
normal
RESOLVED INCOMPLETE
18 years ago
5 years ago

People

(Reporter: dp, Unassigned)

Tracking

({perf})

Trunk
mozilla1.0.1
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts])

Attachments

(1 attachment)

Reporter

Description

18 years ago
nsMemory::Free  52541 calls 0.02ms (without descendents)
Reporter

Comment 1

18 years ago
These calls are at startup.
Blocks: 7251
Keywords: perf
Reporter

Comment 2

18 years ago
And levels of function indirection removed too.
Reporter

Updated

18 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.7
Reporter

Updated

18 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.6
Reporter

Comment 3

18 years ago

Comment 4

18 years ago
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.
Reporter

Comment 5

18 years ago
I am doing timing analysis on this today. Will report timing.

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

Comment 7

18 years ago
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.
Reporter

Comment 8

18 years ago
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.
Reporter

Comment 9

18 years ago

*** This bug has been marked as a duplicate of 75620 ***
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → DUPLICATE

Comment 10

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

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

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

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

-- rick
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: nsMemory::Free needs to be inlined → During startup nsMemory::Free(...) is being called too much !
0.9.6 is long gone. -> 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Reporter

Updated

18 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.8
stacks for short-lived objects?  spacetrace can find them.

/be
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.
Reporter

Comment 17

18 years ago
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.
Status: REOPENED → ASSIGNED
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Reporter

Comment 18

18 years ago
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.
Target Milestone: mozilla0.9.9 → mozilla1.0.1
So this bug should be titled 'Startup creates too many shortlived memory
allocations' ?

Comment 20

15 years ago
Should mlk be added to keywords?
Assignee: dp → nobody
Status: ASSIGNED → NEW
QA Contact: scc → xpcom
is this bug valid anymore?
Whiteboard: [ts]
closing - most recent substantive comments were 7 years ago. -> INCOMPLETE
Status: NEW → RESOLVED
Closed: 18 years ago10 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.