nsPresArena::mFreeLists should be an array rather than a hashtable

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({perf})

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

7 months ago
nsPresArena::mFreeLists is currently a hashtable.  However, most of the uses of it are based on two sets of fixed entries (AllocateByFrameID uses nsQueryFrame::FrameIID and AllocateByObjectID uses mozilla::ArenaObjectID), and AllocateBySize probably uses a relatively limited set of buckets that we could perhaps more strictly limit.

mFreeLists should instead just be an array so that we don't have to deal with a hashtable.

(We could perhaps get rid of AllocateBySize entirely and avoid dealing with the size problem!)

Updated

7 months ago
Rank: 25
Priority: -- → P2
(Assignee)

Updated

7 months ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Rank: 25
Priority: P2 → --
(Assignee)

Updated

7 months ago
Component: Audio/Video: GMP → Layout
(Assignee)

Comment 1

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c34da624ae859c57a4bc81102b06354369a6929&group_state=expanded
(Assignee)

Comment 2

7 months ago
Created attachment 8870582 [details] [diff] [review]
patch 1 - Convert the 4 objects that use nsPresArena::AllocateBySize to use AllocateByObjectID.
Attachment #8870582 - Flags: review?(mats)
Comment hidden (obsolete)
(Assignee)

Comment 4

7 months ago
Created attachment 8870586 [details] [diff] [review]
patch 2 - Remove nsPresArena::AllocateBySize, nsIPresShell::AllocateMisc, and nsPresContext::AllocateFromShell

MozReview-Commit-ID: l2UQC7qOQ0
Attachment #8870586 - Flags: review?(mats)
(Assignee)

Updated

7 months ago
Attachment #8870584 - Attachment is obsolete: true
Attachment #8870584 - Flags: review?(mats)
Comment hidden (obsolete)
(Assignee)

Comment 6

7 months ago
Aargh!  Something in version-control-tools is changing the descriptions of the patches in my queue repository!
(Assignee)

Updated

7 months ago
Attachment #8870590 - Attachment is obsolete: true
Attachment #8870590 - Flags: review?(mats)
Comment hidden (obsolete)
(Assignee)

Updated

7 months ago
Attachment #8870593 - Attachment is obsolete: true
Attachment #8870593 - Flags: review?(mats)
(Assignee)

Comment 8

7 months ago
Created attachment 8870595 [details] [diff] [review]
patch 3 - Store nsPresArena::mFreeLists as an array.
Attachment #8870595 - Flags: review?(mats)
(Assignee)

Comment 9

7 months ago
Also, for the record, mozilla::eArenaObjectID_COUNT is currently 215.

I've reduced FreeList from 4 (64-bit) or 5 (32-bit) words to 3 words, and PLDHashTable's entry store leaves a decent number of free entries (doubling the store size when it reaches 75%, I believe).

I'm not exactly sure what that adds up to memory-wise, but I think it's worth the tradeoff.
(Assignee)

Updated

7 months ago
Attachment #8870595 - Attachment description: Bug 1367190 patch 3 - Store nsPresArena::mFreeLists as an array. → patch 3 - Store nsPresArena::mFreeLists as an array.
(Assignee)

Comment 10

7 months ago
So the memory usage of the PLDHashTable prior to the patch should have been (counting the entry store (20 bytes (32-bit) or 32 bytes (64-bit) per entry) plus the 28-byte (32-bit) or 40-byte (64-bit) PLDHashTable itself) should be (ignoring allocator overhead for the extra allocation in the old setup!), I think:

types     entry     32-bit   64-bit
alloc'd   store     size     size
          entries
    0-6         8     188      296
   7-12        16     348      552
  13-24        32     668     1064
  25-48        64    1308     2088
  49-96       128    2588     4136
 97-192       256    5148     8232
193-384       512   10268    16424

With the new setup, we always allocate 2580 bytes on 32-bit and 5160 bytes on 64-bit.

So on 32-bit the new setup uses less space if we've allocated objects with 49 or more of the types; on 64-bit it uses less space if we've allocated 97 or more.


Loading data:text/html,<!DOCTYPE HTML><body> allocates objects of 36 of the types (with the patch to convert the last 4 types).

Loading https://www.google.com allocates objects of 61 of the types (again, with the conversion patch).


I computed those in gdb with:

set $i = 0
set $sum = 0
while $i < 215
  set $sum = $sum + ($13.mFreeLists[$i].mEntriesEverAllocated != 0)
  set $i = $i + 1
  end
p $i
p $sum
(Assignee)

Comment 11

7 months ago
Oh, and the reason I noticed this was because I saw the hashtable operation show up in a profile (although only on one sample... but it was also a pretty small profile).
Keywords: perf
(Assignee)

Comment 12

7 months ago
https://www.amazon.com/ -> 68
https://bugzilla.mozilla.org/query.cgi (and click on "Advanced") -> 64
http://www.webstandards.org/files/acid2/test.html#top -> 56
https://www.nytimes.com/ -> 68
Comment on attachment 8870582 [details] [diff] [review]
patch 1 - Convert the 4 objects that use nsPresArena::AllocateBySize to use AllocateByObjectID.

> layout/generic/nsIntervalSet.cpp
>+    Interval *newInterval = static_cast<Interval*>(AllocateInterval());

I'd use 'auto' here instead of repeating the type.

>     if (!newInterval) {
>         NS_NOTREACHED("allocation failure");
>         return;
>     }

While we're here... please remove this since pres arena allocation is
infallible now.

> layout/generic/nsIntervalSet.h
> typedef void *
> (* IntervalSetAlloc)(size_t aBytes, void *aClosure);
> 
> typedef void
> (* IntervalSetFree) (size_t aBytes, void *aPtr, void *aClosure);

Can we remove those typedefs now?
Attachment #8870582 - Flags: review?(mats) → review+

Updated

7 months ago
Attachment #8870586 - Flags: review?(mats) → review+
Comment on attachment 8870595 [details] [diff] [review]
patch 3 - Store nsPresArena::mFreeLists as an array.

>layout/base/nsPresArena.cpp
>+  for (FreeList *entry = mFreeLists; entry != ArrayEnd(mFreeLists); ++entry) {

nit: * with the typename please

>   // If there is no free-list entry for this type already, we have
>   // to create one now, to record its size.
>-  FreeList* list = mFreeLists.PutEntry(aCode);
>+  FreeList* list = &mFreeLists[aCode];

The code comment becomes a bit confusing now since the "... we have to
create one now," part comments on the PutEntry() which we now remove.
We should probably remove it altogether.

>   nsTArray<void*>::index_type len = list->mEntries.Length();
>   if (list->mEntrySize == 0) {
>     MOZ_ASSERT(len == 0, "list with entries but no recorded size");
>     list->mEntrySize = aSize;
>   } else {
>     MOZ_ASSERT(list->mEntrySize == aSize,
>                "different sizes for same object type code");

This code can probably be simplified and avoid the branch.  Could we write
the assertion(s) upfront, and then unconditionally assign
"list->mEntrySize = aSize" afterwards?
(feel free to leave it as is though)

>+  for (FreeList *entry = mFreeLists; entry != ArrayEnd(mFreeLists); ++entry) {

nit: * with the typename please

>layout/base/nsPresArena.h
>+    FreeList()
>+      : mEntrySize(0)
>+      , mEntriesEverAllocated(0)
>+    {
>+    }

nit: I think {} is the more commonly used style

>     // Default copy constructor and destructor are ok.

This comment should be removed.
Attachment #8870595 - Flags: review?(mats) → review+
(In reply to David Baron :dbaron: ⌚️UTC-4 from comment #10)
> With the new setup, we always allocate 2580 bytes on 32-bit and 5160 bytes
> on 64-bit.

FYI, I found 3 of the frame IDs we declare are actually not used anymore,
so there will be 3 less entries in the array soon.
(Assignee)

Comment 16

7 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9becc74079ff446688e59470cc38bd75dc3f5c9e
Bug 1367190 patch 1 - Convert the 4 objects that use nsPresArena::AllocateBySize to use AllocateByObjectID.  r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f672dfdeb6bbf77d204b530b0b8f22772a19f5
Bug 1367190 patch 2 - Remove nsPresArena::AllocateBySize, nsIPresShell::AllocateMisc, and nsPresContext::AllocateFromShell.  r=mats

https://hg.mozilla.org/integration/mozilla-inbound/rev/b096ffc589e36bbf87fa28beaef8edce2bd3b31a
Bug 1367190 patch 3 - Store nsPresArena::mFreeLists as an array.  r=mats
(Assignee)

Comment 17

7 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc599cd5dd8342e7469b19e5ed9898751dfad562
Bug 1367190 - Fix bustage on a CLOSED TREE.

Comment 18

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9becc74079ff
https://hg.mozilla.org/mozilla-central/rev/d0f672dfdeb6
https://hg.mozilla.org/mozilla-central/rev/b096ffc589e3
https://hg.mozilla.org/mozilla-central/rev/cc599cd5dd83
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox57: affected → ---
You need to log in before you can comment on or make changes to this bug.