Closed Bug 1341006 Opened 7 years ago Closed 7 years ago

High atoms-mark-bitmaps memory footprint

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- unaffected
firefox54 --- verified

People

(Reporter: bugzilla.mozilla.org, Assigned: bhackett1024)

References

Details

(Keywords: memory-leak, regression, Whiteboard: [MemShrink:P1])

Attachments

(1 file)

Name 	Firefox Version 	54.0a1 Build ID 	20170216030210

From the parent process:

5,354.25 MB (100.0%) -- explicit
├──3,943.54 MB (73.65%) -- js-non-window
│  ├──2,208.52 MB (41.25%) -- runtime
│  │  ├──2,163.13 MB (40.40%) ── atoms-mark-bitmaps
│  │  └─────45.39 MB (00.85%) ++ (13 tiny)
│  ├──1,710.11 MB (31.94%) ++ zones
│  └─────24.92 MB (00.47%) ++ gc-heap
├────481.20 MB (08.99%) ── heap-unclassified
├────477.63 MB (08.92%) ++ window-objects
├────176.39 MB (03.29%) ++ heap-overhead
├─────90.65 MB (01.69%) ++ xpconnect
├─────65.98 MB (01.23%) ++ add-ons
├─────62.26 MB (01.16%) ++ dom
└─────56.60 MB (01.06%) ++ (20 tiny)
Can you reproduce this? Does it happen in safe mode? This looks very bad.
Blocks: 1324002
Flags: needinfo?(bugzilla.mozilla.org)
Whiteboard: [MemShrink]
I have looked at older memory reports I have saved and don't see an atoms-mark-bitmaps there. So it's a new feature or memory report entry? What is its expected behavior, i.e. how with which other parts of the heap is it supposed to scale?


> Can you reproduce this?

Reproduce would not be a word I'd use yet. The report above was the first time I have spotted it, that session had several days of uptime. With today's session I have a second sample:

4,658.56 MB (100.0%) -- explicit
├──2,468.58 MB (52.99%) -- js-non-window
│  ├──1,359.42 MB (29.18%) ++ zones
│  ├──1,083.26 MB (23.25%) -- runtime
│  │  ├──1,041.63 MB (22.36%) ── atoms-mark-bitmaps
│  │  └─────41.63 MB (00.89%) ++ (13 tiny)
│  └─────25.91 MB (00.56%) ++ gc-heap
├────794.38 MB (17.05%) ++ window-objects
├────616.28 MB (13.23%) ── heap-unclassified
├────439.66 MB (09.44%) ++ heap-overhead
├─────92.43 MB (01.98%) ++ dom
├─────90.07 MB (01.93%) ++ (20 tiny)
├─────78.73 MB (01.69%) ++ xpconnect
└─────78.43 MB (01.68%) ++ add-ons


> Does it happen in safe mode? This looks very bad.

Haven't tried.
Flags: needinfo?(bugzilla.mozilla.org)
(In reply to The 8472 from comment #2)
> I have looked at older memory reports I have saved and don't see an
> atoms-mark-bitmaps there. So it's a new feature or memory report entry? What
> is its expected behavior, i.e. how with which other parts of the heap is it
> supposed to scale?

Yes, this was added in bug 1324002. It changed how the memory management of certain kinds of strings (atoms) are handled. The bitmaps are just used to check whether the atoms are alive or not, so I would not expect the memory to go out of control like this. It seems likely there is a leak in them somewhere, given that you don't also have the memory for atoms ballooning up.
That bug states

> Each zone has its own bitmap of atoms which it might reference

So it sounds like it doesn't just scale with the number of strings but also with the zones. I have a lot of lazy tabs and since each about:blank seems to have its own zone that would mean a lot of zones, right? That might explain it. In that case it wouldn't be a leak, just bad scaling.

Although I wonder why it was twice as large in the previous session although the number of tabs was roughly similar.
(In reply to The 8472 from comment #4)
> That bug states
> 
> > Each zone has its own bitmap of atoms which it might reference
> 
> So it sounds like it doesn't just scale with the number of strings but also
> with the zones. I have a lot of lazy tabs and since each about:blank seems
> to have its own zone that would mean a lot of zones, right? That might
> explain it. In that case it wouldn't be a leak, just bad scaling.
> 
> Although I wonder why it was twice as large in the previous session although
> the number of tabs was roughly similar.

How many zones do you have in about:memory?

There are several inefficiencies with the current implementation.  The scaling isn't so much numZones * numAtoms, but numZones * maxAtomsWhichEverSimultaneouslyExisted, which is even worse.

The best way forward here is I think to use a sparse bitmap, so that the amount of memory used by a zone bitmap is linear with the number of atoms it actually references.  That should fix the scaling problems, though there will still be other constant-factor inefficiencies.  I'll try to write a patch in the next day or two.
> How many zones do you have in about:memory?

about 1600. Maybe it would make sense to move about:blanks into a common JS zone, if that's possible?
Summary: Leak of atoms-mark-bitmaps → High atoms-mark-bitmaps memory footprint
Attached patch patchSplinter Review
This patch uses a sparse representation for the bitmaps stored on zones.  The bitmap is divided into page-size blocks, and a hash table is used to keep track of the blocks which a zone has set any bits within.  This shouldn't increase memory usage much at all for bitmaps that are filled in densely, and the main time overhead is a hashtable lookup when setting the bit for a particular atom, which shouldn't have much of an impact.

I wasn't able to reproduce the circumstances described here of having a bazillion zones; when I open a bunch of about:blank tabs or restore a session with a bunch of tabs there aren't many zones that are active.  I did test this with a testcase that produces a lot of atoms, and verified it does keep zone bitmaps much smaller than they would be before this patch.
Assignee: nobody → bhackett1024
Attachment #8840138 - Flags: review?(jcoppeard)
Whiteboard: [MemShrink] → [MemShrink:P1]
Comment on attachment 8840138 [details] [diff] [review]
patch

Review of attachment 8840138 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, and it's nice to see the bitmap manipulation factored out.

The bitwiseOrWith issue below seems concerning, otherwise r=me.

::: js/src/ds/Bitmap.cpp
@@ +11,5 @@
> +SparseBitmap::SparseBitmap()
> +{
> +    AutoEnterOOMUnsafeRegion oomUnsafe;
> +    if (!data.init())
> +        oomUnsafe.crash("Bitmap OOM");

SparseBitmap is initialized as part of the Zone, so we could handle failure here rather than crashing.

@@ +124,5 @@
> +{
> +    size_t blockWord = blockStartWord(wordStart);
> +
> +    // We only support using a single bit block in this API.
> +    MOZ_ASSERT(numWords && (blockWord == blockStartWord(wordStart + numWords - 1)));

Can you change the method name to indicate this restriction?

::: js/src/ds/Bitmap.h
@@ +43,5 @@
> +    uintptr_t& word(size_t i) { return data[i]; }
> +
> +    void bitwiseOrWith(size_t wordStart, size_t numWords, uintptr_t* source) {
> +        MOZ_ASSERT(wordStart + numWords <= data.length());
> +        mozilla::PodCopy(&data[wordStart], source, numWords);

The name says 'or' but this just overwrites the data - is that right?

@@ +72,5 @@
> +    // Return the number of words in a BitBlock starting at |blockWord| which
> +    // are in |other|.
> +    static size_t wordIntersectCount(size_t blockWord, const DenseBitmap& other) {
> +        ssize_t count = other.numWords() - blockWord;
> +        return (count < 0) ? 0 : ((count > (ssize_t)WordsInBlock) ? WordsInBlock : count);

This nested ternary operator is a little confusing to read.  Can you refactor using at least an if statement for the outer condition?
Attachment #8840138 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #8)
> ::: js/src/ds/Bitmap.h
> @@ +43,5 @@
> > +    uintptr_t& word(size_t i) { return data[i]; }
> > +
> > +    void bitwiseOrWith(size_t wordStart, size_t numWords, uintptr_t* source) {
> > +        MOZ_ASSERT(wordStart + numWords <= data.length());
> > +        mozilla::PodCopy(&data[wordStart], source, numWords);
> 
> The name says 'or' but this just overwrites the data - is that right?

Oops, yes, you're right.  The behavior here is correct, I'll change the name to copyBitsFrom().  This is used by computeBitmapFromChunkMarkBits, which starts with an empty dense bitmap and just fills in parts of it from the chunk bits in different arenas.
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b49024a794b
Use a sparse representation for atom mark bitmaps in zones, r=jonco.
https://hg.mozilla.org/mozilla-central/rev/5b49024a794b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Are things looking better for you since this patch landed?
Flags: needinfo?(bugzilla.mozilla.org)
yes, significantly

├──1,723.45 MB (45.07%) -- js-non-window
│  ├──1,563.88 MB (40.90%) ++ zones
│  ├────136.14 MB (03.56%) -- runtime
│  │    ├───79.85 MB (02.09%) ── atoms-mark-bitmaps
│  │    └───56.30 MB (01.47%) ++ (13 tiny)
Flags: needinfo?(bugzilla.mozilla.org)
Thanks for confirming!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.