Closed Bug 1418153 Opened 2 years ago Closed 2 years ago

arena_t refactors, part 1

Categories

(Core :: Memory Allocator, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(8 files)

No description provided.
Comment on attachment 8929255 [details]
Bug 1418153 - Move arena selection one level up the call stack.

https://reviewboard.mozilla.org/r/200536/#review205684


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

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


::: memory/build/mozjemalloc.cpp:4269
(Diff revision 1)
>  
>    if (aSize == 0) {
>      aSize = 1;
>    }
> +  arena = mArena ? mArena : choose_arena(aSize);
> +  ret = imalloc(aSize, /* zero = */ false, arena);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

  ret = imalloc(aSize, /* zero = */ false, arena);
                       ^
                       /* aZero = */

::: memory/build/mozjemalloc.cpp:4314
(Diff revision 1)
>        size_t allocSize = checkedSize.value();
>        if (allocSize == 0) {
>          allocSize = 1;
>        }
> -      ret = imalloc(allocSize, /* zero = */ true, mArena);
> +      arena_t* arena = mArena ? mArena : choose_arena(allocSize);
> +      ret = imalloc(allocSize, /* zero = */ true, arena);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

      ret = imalloc(allocSize, /* zero = */ true, arena);
                               ^
                               /* aZero = */

::: memory/build/mozjemalloc.cpp:4351
(Diff revision 1)
>    } else {
>      if (!malloc_init()) {
>        ret = nullptr;
>      } else {
> -      ret = imalloc(aSize, /* zero = */ false, mArena);
> +      arena_t* arena = mArena ? mArena : choose_arena(aSize);
> +      ret = imalloc(aSize, /* zero = */ false, arena);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

      ret = imalloc(aSize, /* zero = */ false, arena);
                           ^
                           /* aZero = */
Comment on attachment 8929259 [details]
Bug 1418153 - Fold imalloc into arena_t::Malloc.

https://reviewboard.mozilla.org/r/200544/#review205686


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

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


::: memory/build/mozjemalloc.cpp:4261
(Diff revision 1)
>  
>    if (aSize == 0) {
>      aSize = 1;
>    }
>    arena = mArena ? mArena : choose_arena(aSize);
> -  ret = imalloc(aSize, /* zero = */ false, arena);
> +  ret = arena->Malloc(aSize, /* zero = */ false);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

  ret = arena->Malloc(aSize, /* zero = */ false);
                             ^
                             /* aZero = */

::: memory/build/mozjemalloc.cpp:4302
(Diff revision 1)
>        size_t allocSize = checkedSize.value();
>        if (allocSize == 0) {
>          allocSize = 1;
>        }
>        arena_t* arena = mArena ? mArena : choose_arena(allocSize);
> -      ret = imalloc(allocSize, /* zero = */ true, arena);
> +      ret = arena->Malloc(allocSize, /* zero = */ true);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

      ret = arena->Malloc(allocSize, /* zero = */ true);
                                     ^
                                     /* aZero = */

::: memory/build/mozjemalloc.cpp:4339
(Diff revision 1)
>    } else {
>      if (!malloc_init()) {
>        ret = nullptr;
>      } else {
>        arena_t* arena = mArena ? mArena : choose_arena(aSize);
> -      ret = imalloc(aSize, /* zero = */ false, arena);
> +      ret = arena->Malloc(aSize, /* zero = */ false);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

      ret = arena->Malloc(aSize, /* zero = */ false);
                                 ^
                                 /* aZero = */
Comment on attachment 8929261 [details]
Bug 1418153 - Add Large to SizeClass::ClassType.

https://reviewboard.mozilla.org/r/200548/#review205690


C/C++ static analysis found 1 defect in this patch.

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


::: memory/build/mozjemalloc.cpp:3682
(Diff revision 1)
> -      if (aSize < aOldSize) {
> +    if (aSize < aOldSize) {
> -        memset(
> +      memset(
> -          (void*)(uintptr_t(aPtr) + aSize), kAllocPoison, aOldSize - aSize);
> +        (void*)(uintptr_t(aPtr) + aSize), kAllocPoison, aOldSize - aSize);
> -      }
> +    }
> -      return aPtr;
> +    return aPtr;
> +  } else if (sizeClass.Type() == SizeClass::Large &&

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8929255 [details]
Bug 1418153 - Move arena selection one level up the call stack.

https://reviewboard.mozilla.org/r/200536/#review205738

::: memory/build/mozjemalloc.cpp:4268
(Diff revision 1)
>    }
>  
>    if (aSize == 0) {
>      aSize = 1;
>    }
> +  arena = mArena ? mArena : choose_arena(aSize);

Does it make more sense to move it out another level, i.e. disallow null mArena values?
Attachment #8929255 - Flags: review?(n.nethercote) → review+
Comment on attachment 8929256 [details]
Bug 1418153 - Rename arena_t::Palloc to arena_t::PallocLarge.

https://reviewboard.mozilla.org/r/200538/#review205740
Attachment #8929256 - Flags: review?(n.nethercote) → review+
Comment on attachment 8929257 [details]
Bug 1418153 - Move ipalloc to a method of arena_t.

https://reviewboard.mozilla.org/r/200540/#review205742
Attachment #8929257 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #12)
> Comment on attachment 8929255 [details]
> Bug 1418153 - Move arena selection one level up the call stack.
> 
> https://reviewboard.mozilla.org/r/200536/#review205738
> 
> ::: memory/build/mozjemalloc.cpp:4268
> (Diff revision 1)
> >    }
> >  
> >    if (aSize == 0) {
> >      aSize = 1;
> >    }
> > +  arena = mArena ? mArena : choose_arena(aSize);
> 
> Does it make more sense to move it out another level, i.e. disallow null
> mArena values?

That would make it necessary to call choose_arena() in cases we don't need to (realloc and free). Or to expand the DefaultMalloc implementation not to use the macro magic.
Comment on attachment 8929258 [details]
Bug 1418153 - Move huge_malloc/huge_palloc to methods of arena_t.

https://reviewboard.mozilla.org/r/200542/#review205744
Attachment #8929258 - Flags: review?(n.nethercote) → review+
Comment on attachment 8929259 [details]
Bug 1418153 - Fold imalloc into arena_t::Malloc.

https://reviewboard.mozilla.org/r/200544/#review205748
Attachment #8929259 - Flags: review?(n.nethercote) → review+
Comment on attachment 8929260 [details]
Bug 1418153 - Remove impossible condition.

https://reviewboard.mozilla.org/r/200546/#review205752

::: commit-message-120ae:5
(Diff revision 1)
> +Bug 1418153 - Remove impossible condition. r?njn
> +
> +Since the old size arena_ralloc is given is already a size class, when
> +the size class for the new size is the same as the old size, the new
> +size can't be larger than the old size, so there's never anything to

Is it worth adding an assertion to confirm this "can't be larger" fact?

::: memory/build/mozjemalloc.cpp:3698
(Diff revision 1)
>    }
>    return ret;
>  }
>  
>  static void*
>  arena_ralloc(void* aPtr, size_t aSize, size_t aOldSize, arena_t* aArena)

Is it worth changing aOldSize's type to SizeClass, to make it more obvious that it's a size class size?
Attachment #8929260 - Flags: review?(n.nethercote) → review+
Comment on attachment 8929261 [details]
Bug 1418153 - Add Large to SizeClass::ClassType.

https://reviewboard.mozilla.org/r/200548/#review205766
Attachment #8929261 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #18)
> Comment on attachment 8929260 [details]
> Bug 1418153 - Remove impossible condition.
> 
> https://reviewboard.mozilla.org/r/200546/#review205752
> 
> ::: commit-message-120ae:5
> (Diff revision 1)
> > +Bug 1418153 - Remove impossible condition. r?njn
> > +
> > +Since the old size arena_ralloc is given is already a size class, when
> > +the size class for the new size is the same as the old size, the new
> > +size can't be larger than the old size, so there's never anything to
> 
> Is it worth adding an assertion to confirm this "can't be larger" fact?

I don't think so.

> ::: memory/build/mozjemalloc.cpp:3698
> (Diff revision 1)
> >    }
> >    return ret;
> >  }
> >  
> >  static void*
> >  arena_ralloc(void* aPtr, size_t aSize, size_t aOldSize, arena_t* aArena)
> 
> Is it worth changing aOldSize's type to SizeClass, to make it more obvious
> that it's a size class size?

That's actually something I've been toying with. That's something I'll put in arena_t refactors, part 2 or some other upcoming bug.
(In reply to Mike Hommey [:glandium] from comment #20)
> That's actually something I've been toying with.

(more generally than just here)
Comment on attachment 8929262 [details]
Bug 1418153 - Move arena_ralloc/huge_ralloc to methods of arena_t.

https://reviewboard.mozilla.org/r/200550/#review205798
Attachment #8929262 - Flags: review?(n.nethercote) → review+
Comment on attachment 8929255 [details]
Bug 1418153 - Move arena selection one level up the call stack.

https://reviewboard.mozilla.org/r/200536/#review205940


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

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


::: memory/build/mozjemalloc.cpp:4269
(Diff revision 2)
>  
>    if (aSize == 0) {
>      aSize = 1;
>    }
> +  arena = mArena ? mArena : choose_arena(aSize);
> +  ret = imalloc(aSize, /* zero = */ false, arena);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

  ret = imalloc(aSize, /* zero = */ false, arena);
                       ^
                       /* aZero = */

::: memory/build/mozjemalloc.cpp:4314
(Diff revision 2)
>        size_t allocSize = checkedSize.value();
>        if (allocSize == 0) {
>          allocSize = 1;
>        }
> -      ret = imalloc(allocSize, /* zero = */ true, mArena);
> +      arena_t* arena = mArena ? mArena : choose_arena(allocSize);
> +      ret = imalloc(allocSize, /* zero = */ true, arena);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

      ret = imalloc(allocSize, /* zero = */ true, arena);
                               ^
                               /* aZero = */

::: memory/build/mozjemalloc.cpp:4351
(Diff revision 2)
>    } else {
>      if (!malloc_init()) {
>        ret = nullptr;
>      } else {
> -      ret = imalloc(aSize, /* zero = */ false, mArena);
> +      arena_t* arena = mArena ? mArena : choose_arena(aSize);
> +      ret = imalloc(aSize, /* zero = */ false, arena);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

      ret = imalloc(aSize, /* zero = */ false, arena);
                           ^
                           /* aZero = */
Comment on attachment 8929259 [details]
Bug 1418153 - Fold imalloc into arena_t::Malloc.

https://reviewboard.mozilla.org/r/200544/#review205946


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

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


::: memory/build/mozjemalloc.cpp:4261
(Diff revision 2)
>  
>    if (aSize == 0) {
>      aSize = 1;
>    }
>    arena = mArena ? mArena : choose_arena(aSize);
> -  ret = imalloc(aSize, /* zero = */ false, arena);
> +  ret = arena->Malloc(aSize, /* zero = */ false);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

  ret = arena->Malloc(aSize, /* zero = */ false);
                             ^
                             /* aZero = */

::: memory/build/mozjemalloc.cpp:4302
(Diff revision 2)
>        size_t allocSize = checkedSize.value();
>        if (allocSize == 0) {
>          allocSize = 1;
>        }
>        arena_t* arena = mArena ? mArena : choose_arena(allocSize);
> -      ret = imalloc(allocSize, /* zero = */ true, arena);
> +      ret = arena->Malloc(allocSize, /* zero = */ true);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

      ret = arena->Malloc(allocSize, /* zero = */ true);
                                     ^
                                     /* aZero = */

::: memory/build/mozjemalloc.cpp:4339
(Diff revision 2)
>    } else {
>      if (!malloc_init()) {
>        ret = nullptr;
>      } else {
>        arena_t* arena = mArena ? mArena : choose_arena(aSize);
> -      ret = imalloc(aSize, /* zero = */ false, arena);
> +      ret = arena->Malloc(aSize, /* zero = */ false);

Warning: Argument name 'zero' in comment does not match parameter name 'azero' [clang-tidy: misc-argument-comment]

      ret = arena->Malloc(aSize, /* zero = */ false);
                                 ^
                                 /* aZero = */
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/42ad440d0c5f
Move arena selection one level up the call stack. r=njn
https://hg.mozilla.org/integration/autoland/rev/d4967677f74f
Rename arena_t::Palloc to arena_t::PallocLarge. r=njn
https://hg.mozilla.org/integration/autoland/rev/f28c574c46da
Move ipalloc to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/0ba285599096
Move huge_malloc/huge_palloc to methods of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/cd581e9fd969
Fold imalloc into arena_t::Malloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/8626690c488d
Remove impossible condition. r=njn
https://hg.mozilla.org/integration/autoland/rev/d29db33af7e5
Add Large to SizeClass::ClassType. r=njn
https://hg.mozilla.org/integration/autoland/rev/33016a4ef54e
Move arena_ralloc/huge_ralloc to methods of arena_t. r=njn
You need to log in before you can comment on or make changes to this bug.