1413096 - Miscellaneous code cleanups in mozjemalloc.cpp

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1413096 - Add missing header guard.

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

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1413096 - Add "using namespace mozilla" to mozjemalloc.cpp.

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

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1413096 - Use CheckedInt for overflow check in calloc.

Also use in in _recalloc.

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

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1413096 - Remove pow2_ceil in favor of RoundUpPow2.

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

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1413096 - Remove unnecessary call to ffs.

Comparing ffs(x) == ffs(y), when x and y are guaranteed to be powers of
2 (or 0, or 1), is the same as x == y.

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

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1413096 - Replace ffs with MathAlgorithms functions.

- In the cases where it's used on powers of 2, replace it with FloorLog2
- In the cases where it's used on any kind of number, replace it with
  CountTrailingZeroes, which is `ffs(x) - 1`.

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

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 1413096 - Remove SIZEOF_PTR and SIZEOF_PTR_2POW.

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

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8923695 [details]
Bug 1413096 - Replace ffs with MathAlgorithms functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194826/diff/1-2/

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8923696 [details]
Bug 1413096 - Remove SIZEOF_PTR and SIZEOF_PTR_2POW.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194828/diff/1-2/

Comment 10

[Mozilla Autoland]

hg error in cmd: hg update --clean -r afc2cbb9370f: abort: unknown revision 'afc2cbb9370f'!

Comment 11

[Mike Hommey [:glandium]]

Attachment: Bug 1413096 - Remove compile-time constants from data returned by jemalloc_stats.

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

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8923690 [details]
Bug 1413096 - Add missing header guard.

https://reviewboard.mozilla.org/r/194816/#review200270

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8923691 [details]
Bug 1413096 - Add "using namespace mozilla" to mozjemalloc.cpp.

https://reviewboard.mozilla.org/r/194818/#review200272

Nice.

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 8923692 [details]
Bug 1413096 - Use CheckedInt for overflow check in calloc.

https://reviewboard.mozilla.org/r/194820/#review200274

Comment 15

[Nicholas Nethercote [inactive]]

Comment on attachment 8923693 [details]
Bug 1413096 - Remove pow2_ceil in favor of RoundUpPow2.

https://reviewboard.mozilla.org/r/194822/#review200276

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 8923694 [details]
Bug 1413096 - Remove unnecessary call to ffs.

https://reviewboard.mozilla.org/r/194824/#review200278

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8923695 [details]
Bug 1413096 - Replace ffs with MathAlgorithms functions.

https://reviewboard.mozilla.org/r/194826/#review200280

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8923696 [details]
Bug 1413096 - Remove SIZEOF_PTR and SIZEOF_PTR_2POW.

https://reviewboard.mozilla.org/r/194828/#review200282

::: memory/build/rb.h:716
(Diff revision 2)
>     */
>  public:
>    class Iterator
>    {
> -    TreeNode* mPath[3 * ((SIZEOF_PTR << 3) - (SIZEOF_PTR_2POW + 1))];
> +    TreeNode* mPath[3 * ((sizeof(void*) << 3) -
> +                         (mozilla::tl::CeilingLog2<sizeof(void*)>::value + 1))];

If you move Log2/LOG2 into rb.h you can use them here. Alternatively, you could remove them and just using tl::CeilingLog2 everywhere; I don't feel like the check done by Log2 is that important.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #18)
> Comment on attachment 8923696 [details]
> Bug 1413096 - Remove SIZEOF_PTR and SIZEOF_PTR_2POW.
> 
> https://reviewboard.mozilla.org/r/194828/#review200282
> 
> ::: memory/build/rb.h:716
> (Diff revision 2)
> >     */
> >  public:
> >    class Iterator
> >    {
> > -    TreeNode* mPath[3 * ((SIZEOF_PTR << 3) - (SIZEOF_PTR_2POW + 1))];
> > +    TreeNode* mPath[3 * ((sizeof(void*) << 3) -
> > +                         (mozilla::tl::CeilingLog2<sizeof(void*)>::value + 1))];
> 
> If you move Log2/LOG2 into rb.h you can use them here. Alternatively, you
> could remove them and just using tl::CeilingLog2 everywhere; I don't feel
> like the check done by Log2 is that important.

I went with a macro because tl::CeilingLog2<foo>::value is a lot of verbosity. How about moving Log2/LOG2 to a Utils.h header?

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8923690 [details]
Bug 1413096 - Add missing header guard.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194816/diff/1-2/

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8923691 [details]
Bug 1413096 - Add "using namespace mozilla" to mozjemalloc.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194818/diff/1-2/

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8923692 [details]
Bug 1413096 - Use CheckedInt for overflow check in calloc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194820/diff/1-2/

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8923693 [details]
Bug 1413096 - Remove pow2_ceil in favor of RoundUpPow2.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194822/diff/1-2/

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8923694 [details]
Bug 1413096 - Remove unnecessary call to ffs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194824/diff/1-2/

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8923695 [details]
Bug 1413096 - Replace ffs with MathAlgorithms functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194826/diff/2-3/

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8923696 [details]
Bug 1413096 - Remove SIZEOF_PTR and SIZEOF_PTR_2POW.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194828/diff/2-3/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6543990db9d9
Add missing header guard. r=njn
https://hg.mozilla.org/integration/autoland/rev/96f7edfa1fa2
Add "using namespace mozilla" to mozjemalloc.cpp. r=njn
https://hg.mozilla.org/integration/autoland/rev/09890a442cf8
Use CheckedInt for overflow check in calloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/cce540baf354
Remove pow2_ceil in favor of RoundUpPow2. r=njn
https://hg.mozilla.org/integration/autoland/rev/1011d19677c2
Remove unnecessary call to ffs. r=njn
https://hg.mozilla.org/integration/autoland/rev/81ce645a6e48
Replace ffs with MathAlgorithms functions. r=njn
https://hg.mozilla.org/integration/autoland/rev/42f477976fc5
Remove SIZEOF_PTR and SIZEOF_PTR_2POW. r=njn

Comment 28

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6543990db9d9
https://hg.mozilla.org/mozilla-central/rev/96f7edfa1fa2
https://hg.mozilla.org/mozilla-central/rev/09890a442cf8
https://hg.mozilla.org/mozilla-central/rev/cce540baf354
https://hg.mozilla.org/mozilla-central/rev/1011d19677c2
https://hg.mozilla.org/mozilla-central/rev/81ce645a6e48
https://hg.mozilla.org/mozilla-central/rev/42f477976fc5