Closed Bug 1413096 Opened 7 years ago Closed 7 years ago

Miscellaneous code cleanups in mozjemalloc.cpp

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla58

Tracking Flags:

Tracking

Status

firefox58

---

fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(7 files, 1 obsolete file)

Bug 1413096 - Add missing header guard. 7 years ago Mike Hommey [:glandium] 59 bytes, text/x-review-board-request	n.nethercote : review+	Details
Bug 1413096 - Add "using namespace mozilla" to mozjemalloc.cpp. 7 years ago Mike Hommey [:glandium] 59 bytes, text/x-review-board-request	n.nethercote : review+	Details
Bug 1413096 - Use CheckedInt for overflow check in calloc. 7 years ago Mike Hommey [:glandium] 59 bytes, text/x-review-board-request	n.nethercote : review+	Details
Bug 1413096 - Remove pow2_ceil in favor of RoundUpPow2. 7 years ago Mike Hommey [:glandium] 59 bytes, text/x-review-board-request	n.nethercote : review+	Details
Bug 1413096 - Remove unnecessary call to ffs. 7 years ago Mike Hommey [:glandium] 59 bytes, text/x-review-board-request	n.nethercote : review+	Details
Bug 1413096 - Replace ffs with MathAlgorithms functions. 7 years ago Mike Hommey [:glandium] 59 bytes, text/x-review-board-request	n.nethercote : review+	Details
Bug 1413096 - Remove SIZEOF_PTR and SIZEOF_PTR_2POW. 7 years ago Mike Hommey [:glandium] 59 bytes, text/x-review-board-request	n.nethercote : review+	Details
Bug 1413096 - Remove compile-time constants from data returned by jemalloc_stats. 7 years ago Mike Hommey [:glandium] 59 bytes, text/x-review-board-request		Details

Mike Hommey [:glandium]

Assignee

Description

•

7 years ago

      No description provided.

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

- 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 hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Mozilla Autoland

Comment 10

•

7 years ago

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

Comment hidden (mozreview-request)

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

Mike Hommey [:glandium]

Assignee

Updated

•

7 years ago

Attachment #8923711 - Attachment is obsolete: true

Attachment #8923711 - Flags: review?(n.nethercote)

Nicholas Nethercote [inactive]

Comment 12

•

7 years ago

mozreview-review

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

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

Attachment #8923690 - Flags: review?(n.nethercote) → review+

Nicholas Nethercote [inactive]

Comment 13

•

7 years ago

mozreview-review

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

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

Nice.

Attachment #8923691 - Flags: review?(n.nethercote) → review+

Nicholas Nethercote [inactive]

Comment 14

•

7 years ago

mozreview-review

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

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

Attachment #8923692 - Flags: review?(n.nethercote) → review+

Nicholas Nethercote [inactive]

Comment 15

•

7 years ago

mozreview-review

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

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

Attachment #8923693 - Flags: review?(n.nethercote) → review+

Nicholas Nethercote [inactive]

Comment 16

•

7 years ago

mozreview-review

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

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

Attachment #8923694 - Flags: review?(n.nethercote) → review+

Nicholas Nethercote [inactive]

Comment 17

•

7 years ago

mozreview-review

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

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

Attachment #8923695 - Flags: review?(n.nethercote) → review+

Nicholas Nethercote [inactive]

Comment 18

•

7 years ago

mozreview-review

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.

Attachment #8923696 - Flags: review?(n.nethercote) → review+

Mike Hommey [:glandium]

Assignee

Comment 19

•

7 years ago

(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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Pulsebot

Comment 27

•

7 years ago

Pushed by mh@glandium.org:
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

Natalia Csoregi [:nataliaCs]

Comment 28

•

7 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox58: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla58

Joel Maher ( :jmaher ) (UTC -8)

Updated

•

7 years ago

Depends on: 1413570

You need to log in before you can comment on or make changes to this bug.