Closed Bug 1402283 Opened 7 years ago Closed 7 years ago

Enforce arenas to match on moz_arena_{realloc,free}

Categories

(Core :: Memory Allocator, enhancement, P1)

Product:
Core
Component:
Memory Allocator
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(5 files)

Bug 1402283 - Rename extent_node_t fields.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1402283 - Make arena_ralloc use the same arena as the original pointer when none is provided.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1402283 - Replace isalloc/isalloc_validate with static methods of a new AllocInfo class.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1402283 - Associate an arena to each huge allocation.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1402283 - Enforce arena matching on moz_arena_{realloc,free}.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
      No description provided.
Assignee: nobody → mh+mozilla
Comment on attachment 8926722 [details]
Bug 1402283 - Rename extent_node_t fields.

https://reviewboard.mozilla.org/r/197956/#review203208
Attachment #8926722 - Flags: review?(n.nethercote) → review+
Comment on attachment 8926723 [details]
Bug 1402283 - Make arena_ralloc use the same arena as the original pointer when none is provided.

https://reviewboard.mozilla.org/r/197958/#review203210
Attachment #8926723 - Flags: review?(n.nethercote) → review+
Comment on attachment 8926724 [details]
Bug 1402283 - Replace isalloc/isalloc_validate with static methods of a new AllocInfo class.

https://reviewboard.mozilla.org/r/197960/#review203212
Attachment #8926724 - Flags: review?(n.nethercote) → review+
Comment on attachment 8926725 [details]
Bug 1402283 - Associate an arena to each huge allocation.

https://reviewboard.mozilla.org/r/197962/#review203214

::: memory/build/mozjemalloc.cpp:632
(Diff revision 1)
>    void* mAddr;
>  
>    // Total region size.
>    size_t mSize;
>  
> +  union {

It would be nice to have a comment here explaining when the different parts of the union are valid.

::: memory/build/mozjemalloc.cpp:3343
(Diff revision 1)
> +  arena_t* Arena()
> +  {
> +    if (mSize <= gMaxLargeClass) {
> +      return mChunk->arena;
> +    }
> +    return mNode->mArena;

You could use ?: here.

::: memory/build/mozjemalloc.cpp:3348
(Diff revision 1)
> +    return mNode->mArena;
> +  }
> +
>  private:
>    size_t mSize;
> +  union {

ditto
Attachment #8926725 - Flags: review?(n.nethercote) → review+
Comment on attachment 8926726 [details]
Bug 1402283 - Enforce arena matching on moz_arena_{realloc,free}.

https://reviewboard.mozilla.org/r/197964/#review203218

::: memory/build/Utils.h:50
(Diff revision 5)
> +constexpr size_t operator"" _MiB(unsigned long long int aNum)
> +{
> +  return size_t(aNum) * 1024_KiB;
> +}
> +
> +constexpr size_t operator"" _MiB(long double aNum)

What's the rounding behaviour here? Might be worth describing in a comment.

::: memory/build/mozjemalloc.cpp:1260
(Diff revision 5)
>  static void*
>  huge_palloc(size_t aSize, size_t aAlignment, bool aZero, arena_t* aArena);
>  static void*
>  huge_ralloc(void* aPtr, size_t aSize, size_t aOldSize, arena_t* aArena);
>  static void
> -huge_dalloc(void* aPtr);
> +huge_dalloc(void* aPtr, arena_t* aArena=nullptr);

Nit: spaces around '='? (Or does clang-format remove them?)

::: memory/gtest/TestJemalloc.cpp:278
(Diff revision 5)
> +  // For convenience, realloc can also be used to reallocate arena pointers.
> +  // The result should be in the same arena. Test various size class transitions.
> +  size_t sizes[] = { 1, 42, 80, 1_KiB, 1.5_KiB, 72_KiB, 129_KiB, 2.5_MiB, 5.1_MiB };
> +  for (size_t from_size : sizes) {
> +    for (size_t to_size : sizes) {
> +      if (from_size != to_size) {

I'd remove this condition and test the same size cases as well.
Attachment #8926726 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #13)
> It would be nice to have a comment here explaining when the different parts
> of the union are valid.

Maybe it's not entirely clear, but "used by chunk recycling code" and "for huge allocations" does describe when the fields are valid.

> ::: memory/build/mozjemalloc.cpp:1260
> (Diff revision 5)
> >  static void*
> >  huge_palloc(size_t aSize, size_t aAlignment, bool aZero, arena_t* aArena);
> >  static void*
> >  huge_ralloc(void* aPtr, size_t aSize, size_t aOldSize, arena_t* aArena);
> >  static void
> > -huge_dalloc(void* aPtr);
> > +huge_dalloc(void* aPtr, arena_t* aArena=nullptr);
> 
> Nit: spaces around '='? (Or does clang-format remove them?)

Err, I didn't run this one against clang-format, and in fact, the default value is not necessary.
(In reply to Nicholas Nethercote [:njn] from comment #14)
> > +constexpr size_t operator"" _MiB(long double aNum)
> 
> What's the rounding behaviour here? Might be worth describing in a comment.

I'm not sure what could be said in a comment that is not clear from the one line of code that is just below.
Flags: needinfo?(n.nethercote)
> I'm not sure what could be said in a comment that is not clear from the one
> line of code that is just below.

Depends how well you remember C++'s rounding rules :)  I think it truncates in this case?
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #17)
> > I'm not sure what could be said in a comment that is not clear from the one
> > line of code that is just below.
> 
> Depends how well you remember C++'s rounding rules :)  I think it truncates
> in this case?

It does. That however doesn't matter much unless you're passing something that you intend to be precise to the byte. On a KiB or a MiB...
(In reply to Nicholas Nethercote [:njn] from comment #14)
> ::: memory/gtest/TestJemalloc.cpp:278
> (Diff revision 5)
> > +  // For convenience, realloc can also be used to reallocate arena pointers.
> > +  // The result should be in the same arena. Test various size class transitions.
> > +  size_t sizes[] = { 1, 42, 80, 1_KiB, 1.5_KiB, 72_KiB, 129_KiB, 2.5_MiB, 5.1_MiB };
> > +  for (size_t from_size : sizes) {
> > +    for (size_t to_size : sizes) {
> > +      if (from_size != to_size) {
> 
> I'd remove this condition and test the same size cases as well.

This leads to an interesting question: should moz_arena_realloc fail when it doesn't do anything (in-place realloc) but the arena it's given is wrong. I think it probably should. What do you think?
Flags: needinfo?(n.nethercote)
It actually makes things simpler.
Flags: needinfo?(n.nethercote)
> This leads to an interesting question: should moz_arena_realloc fail when it
> doesn't do anything (in-place realloc) but the arena it's given is wrong. I
> think it probably should. What do you think?

realloc() is a function that leads to many interesting questions :)

I agree that failing is the right thing. Worth documenting in a comment.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2df7f0bf182
Rename extent_node_t fields. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f44c449e280
Make arena_ralloc use the same arena as the original pointer when none is provided. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0e82c6b316
Replace isalloc/isalloc_validate with static methods of a new AllocInfo class. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/a82bf86d86fd
Associate an arena to each huge allocation. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee9d4052e949
Enforce arena matching on moz_arena_{realloc,free}. r=njn
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/50d59c3a291b
Fix non-Nightly build bustage. r+a=RyanVM
Depends on: 1416473
No longer depends on: 1416473
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: