1415454 - Miscellaneous code cleanups in mozjemalloc.cpp

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1415454 - Inline MallocBinEasy and MallocBinHard.

They are both only used once, are trivial wrappers, and even repeat the
same assertions.

Review commit: https://reviewboard.mozilla.org/r/197550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197550/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1415454 - Remove the unused arena_bin_t* argument to arena_t::AllocRun.

Review commit: https://reviewboard.mozilla.org/r/197552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197552/

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1415454 - Replace log2 lookup table with FloorLog2.

FloorLog2 expands to, essentially, a compiler builtin/intrinsic, that,
in turn, expands to a single machine instruction on tier 1 and other
platforms. On platforms where that's not the case, we can expect the
compiler to generate fast code anyways. So overall, this is all better
than manually using a log2 lookup table.

Also replace a manual power-of-two check with mozilla::IsPowerOfTwo,
which does the same test.

Review commit: https://reviewboard.mozilla.org/r/197554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197554/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1415454 - Replace RedBlackTree::Init with a constructor.

Before 1412722, RedBlackTree actually needed a constructor to fill the
sentinel, but now, all it contains is a pointer to a root node, and an
empty tree just has a null root node. So we can use a constexpr
constructor for the class, which in most cases removes the need for
manual initialization/construction (being used as member of constructed
classes, or globals), except in the case of arena_bin_t, which is never
constructed.

Review commit: https://reviewboard.mozilla.org/r/197556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197556/

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8926281 [details]
Bug 1415454 - Remove the unused arena_bin_t* argument to arena_t::AllocRun.

https://reviewboard.mozilla.org/r/197552/#review202808


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8926281 [details]
Bug 1415454 - Remove the unused arena_bin_t* argument to arena_t::AllocRun.

https://reviewboard.mozilla.org/r/197552/#review202810


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8926285 [details]
Bug 1415454 - Replace RedBlackTree::Init with a constructor.

https://reviewboard.mozilla.org/r/197556/#review202812


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

Comment 8

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #4)
> Created attachment 8926285 [details]
> Bug 1415454 - Replace RedBlackTree::Init with a constructor.
> 
> Before 1412722, RedBlackTree actually needed a constructor to fill the
> sentinel, but now, all it contains is a pointer to a root node, and an
> empty tree just has a null root node. So we can use a constexpr
> constructor for the class, which in most cases removes the need for
> manual initialization/construction (being used as member of constructed
> classes, or globals), except in the case of arena_bin_t, which is never
> constructed.
> 
> Review commit: https://reviewboard.mozilla.org/r/197556/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/197556/

FWIW, this made gcc emit a static initializers for gArenas... to set mArenas.mRoot and mPrivateArenas.mRoot to null... except that's too late because other code has already called malloc before the static initializer runs, so it actually resets both trees...

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8926281 [details]
Bug 1415454 - Remove the unused arena_bin_t* argument to arena_t::AllocRun.

https://reviewboard.mozilla.org/r/197552/#review203112

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8926282 [details]
Bug 1415454 - Replace log2 lookup table with FloorLog2.

https://reviewboard.mozilla.org/r/197554/#review203116

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8926280 [details]
Bug 1415454 - Inline MallocBinEasy and MallocBinHard.

https://reviewboard.mozilla.org/r/197550/#review203120

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f64defe0d11e
Inline MallocBinEasy and MallocBinHard. r=njn
https://hg.mozilla.org/integration/autoland/rev/b8acdda17818
Remove the unused arena_bin_t* argument to arena_t::AllocRun. r=njn
https://hg.mozilla.org/integration/autoland/rev/bfa36a25eaf6
Replace log2 lookup table with FloorLog2. r=njn

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f64defe0d11e
https://hg.mozilla.org/mozilla-central/rev/b8acdda17818
https://hg.mozilla.org/mozilla-central/rev/bfa36a25eaf6