1402283 - Enforce arenas to match on moz_arena_{realloc,free}

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement, P1)

Comment 1

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Bug 1402283 - Rename extent_node_t fields.

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

Comment 2

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Bug 1402283 - Make arena_ralloc use the same arena as the original pointer when none is provided.

When using plain realloc() on a pointer that was allocated with
moz_arena_malloc, we want the resulting pointer to still belong to the
same arena.

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

Comment 3

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Bug 1402283 - Replace isalloc/isalloc_validate with static methods of a new AllocInfo class.

Both functions do essentially the same thing, one having more validation
than the other. We can use a template with a boolean parameter to avoid
the duplication.

Furthermore, we're soon going to require, in some cases, more
information than just the size of the allocation, so we wrap their
result in a helper class that gives information about an active
allocation.

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

Comment 4

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Bug 1402283 - Associate an arena to each huge allocation.

Currently, huge allocations are completely independent from arenas. But
in order to ensure that e.g. moz_arena_realloc can't reallocate huge
allocations from another arena, we need to track which arena was
responsible for the huge allocation. We do that in the corresponding
extent_node_t.

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

Comment 5

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Bug 1402283 - Enforce arena matching on moz_arena_{realloc,free}.

This enforces the API contract as described in memory/build/malloc_decls.h.

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

Comment 6

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8926726 [details]
Bug 1402283 - Enforce arena matching on moz_arena_{realloc,free}.

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

Comment 7

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8926726 [details]
Bug 1402283 - Enforce arena matching on moz_arena_{realloc,free}.

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

Comment 8

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8926726 [details]
Bug 1402283 - Enforce arena matching on moz_arena_{realloc,free}.

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

Comment 9

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8926726 [details]
Bug 1402283 - Enforce arena matching on moz_arena_{realloc,free}.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197964/diff/4-5/

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8926722 [details]
Bug 1402283 - Rename extent_node_t fields.

https://reviewboard.mozilla.org/r/197956/#review203208

Comment 11

[Nicholas Nethercote [inactive]]

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

Comment 12

[Nicholas Nethercote [inactive]]

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

Comment 13

[Nicholas Nethercote [inactive]]

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

Comment 14

[Nicholas Nethercote [inactive]]

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.

Comment 15

[Mike Hommey [:glandium] OoO until Dec 30]

(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.

Comment 16

[Mike Hommey [:glandium] OoO until Dec 30]

(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.

Comment 17

[Nicholas Nethercote [inactive]]

> 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?

Comment 18

[Mike Hommey [:glandium] OoO until Dec 30]

(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...

Comment 19

[Mike Hommey [:glandium] OoO until Dec 30]

(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?

Comment 20

[Mike Hommey [:glandium] OoO until Dec 30]

It actually makes things simpler.

Comment 21

[Nicholas Nethercote [inactive]]

> 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.

Comment 22

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8926722 [details]
Bug 1402283 - Rename extent_node_t fields.

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

Comment 23

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8926723 [details]
Bug 1402283 - Make arena_ralloc use the same arena as the original pointer when none is provided.

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

Comment 24

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8926724 [details]
Bug 1402283 - Replace isalloc/isalloc_validate with static methods of a new AllocInfo class.

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

Comment 25

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8926725 [details]
Bug 1402283 - Associate an arena to each huge allocation.

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

Comment 26

[Mike Hommey [:glandium] OoO until Dec 30]

Comment on attachment 8926726 [details]
Bug 1402283 - Enforce arena matching on moz_arena_{realloc,free}.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197964/diff/5-6/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/e2df7f0bf182
https://hg.mozilla.org/mozilla-central/rev/3f44c449e280
https://hg.mozilla.org/mozilla-central/rev/6e0e82c6b316
https://hg.mozilla.org/mozilla-central/rev/a82bf86d86fd
https://hg.mozilla.org/mozilla-central/rev/ee9d4052e949

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/50d59c3a291b
Fix non-Nightly build bustage. r+a=RyanVM

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/c32f513af0f8
Fix typo. r+a=RyanVM