Miscellaneous code cleanups in mozjemalloc.cpp

RESOLVED FIXED in Firefox 58

Status

()

Core
Memory Allocator
RESOLVED FIXED
20 days ago
18 days ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

20 days ago
hg error in cmd: hg update --clean -r afc2cbb9370f: abort: unknown revision 'afc2cbb9370f'!
Comment hidden (mozreview-request)
(Assignee)

Updated

20 days ago
Attachment #8923711 - Attachment is obsolete: true
Attachment #8923711 - Flags: review?(n.nethercote)

Comment 12

19 days 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+

Comment 13

19 days 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+

Comment 14

19 days 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+

Comment 15

19 days 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+

Comment 16

19 days 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+

Comment 17

19 days 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+

Comment 18

19 days 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+
(Assignee)

Comment 19

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

Comment 27

19 days 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

Comment 28

19 days 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
Last Resolved: 19 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1413570
You need to log in before you can comment on or make changes to this bug.