Closed Bug 1418104 Opened 6 years ago Closed 6 years ago

Add various realloc, junk and poisoning unit tests

Categories

(Core :: Memory Allocator, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(3 files)

      No description provided.
Comment on attachment 8929206 [details]
Bug 1418104 - Allow to pass parameters when creating arenas.

https://reviewboard.mozilla.org/r/200488/#review205622


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

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


::: memory/build/mozjemalloc.cpp:1104
(Diff revision 1)
>  public:
>    bool Init()
>    {
>      mArenas.Init();
>      mPrivateArenas.Init();
> -    mDefaultArena =
> +    mDefaultArena = mLock.Init() ? CreateArena(/* IsPrivate = */ false,

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

    mDefaultArena = mLock.Init() ? CreateArena(/* IsPrivate = */ false,
                                               ^
                                               /* aIsPrivate = */

::: memory/build/mozjemalloc.cpp:1105
(Diff revision 1)
>    bool Init()
>    {
>      mArenas.Init();
>      mPrivateArenas.Init();
> -    mDefaultArena =
> -      mLock.Init() ? CreateArena(/* IsPrivate = */ false) : nullptr;
> +    mDefaultArena = mLock.Init() ? CreateArena(/* IsPrivate = */ false,
> +                                               /* Params = */ nullptr)

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

                                               /* Params = */ nullptr)
                                               ^
                                               /* aParams = */

::: memory/build/mozjemalloc.cpp:2223
(Diff revision 1)
>      // with `false`, except maybe at shutdown.
> -    arena = gArenas.CreateArena(/* IsPrivate = */ false);
> +    arena_params_t params;
> +    // Reduce the maximum amount of dirty pages we allow to be kept on
> +    // thread local arenas. TODO: make this more flexible.
> +    params.mMaxDirty = opt_dirty_max / 8;
> +    arena = gArenas.CreateArena(/* IsPrivate = */ false, &params);

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

    arena = gArenas.CreateArena(/* IsPrivate = */ false, &params);
                                ^
                                /* aIsPrivate = */

::: memory/build/mozjemalloc.cpp:4687
(Diff revision 1)
>  template<>
>  inline arena_id_t
> -MozJemalloc::moz_create_arena()
> +MozJemalloc::moz_create_arena_with_params(arena_params_t* aParams)
>  {
>    if (malloc_init()) {
> -    arena_t* arena = gArenas.CreateArena(/* IsPrivate = */ true);
> +    arena_t* arena = gArenas.CreateArena(/* IsPrivate = */ true, aParams);

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

    arena_t* arena = gArenas.CreateArena(/* IsPrivate = */ true, aParams);
                                         ^
                                         /* aIsPrivate = */
Blocks: 1418123
Comment on attachment 8929205 [details]
Bug 1418104 - Initialize the allocator in jemalloc_stats.

https://reviewboard.mozilla.org/r/200486/#review205646
Attachment #8929205 - Flags: review?(n.nethercote) → review+
Comment on attachment 8929206 [details]
Bug 1418104 - Allow to pass parameters when creating arenas.

https://reviewboard.mozilla.org/r/200488/#review205652

As discussed on IRC, this changes mMaxDirty for custom arenas.

::: memory/build/mozjemalloc_types.h:62
(Diff revision 1)
>  typedef size_t arena_id_t;
>  
> +typedef struct
> +{
> +  size_t mMaxDirty;
> +} arena_params_t;

Should this have a constructor? Manual initialization feels error-prone.
Attachment #8929206 - Flags: review?(n.nethercote) → review-
Comment on attachment 8929207 [details]
Bug 1418104 - Add various realloc, junk and poisoning unit tests.

https://reviewboard.mozilla.org/r/200490/#review205654

::: memory/gtest/TestJemalloc.cpp:342
(Diff revision 1)
> +      }
> +    }
> +  }
> +}
> +
> +// An range iterator for size classes between two given values

Nit: s/An/A/. And '.' at end of sentence.

::: memory/gtest/TestJemalloc.cpp:383
(Diff revision 1)
> +
> +#define ALIGNMENT_CEILING(s, alignment)                                        \
> +  (((s) + (alignment - 1)) & (~(alignment - 1)))
> +
> +static bool
> +IsSameRoundedHugeClass(size_t aSize1, size_t aSize2, jemalloc_stats_t& aStats)

I'd probably pass in large_max and chunksize directly, rather than via aStats.
Attachment #8929207 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #7)
> ::: memory/gtest/TestJemalloc.cpp:383
> (Diff revision 1)
> > +
> > +#define ALIGNMENT_CEILING(s, alignment)                                        \
> > +  (((s) + (alignment - 1)) & (~(alignment - 1)))
> > +
> > +static bool
> > +IsSameRoundedHugeClass(size_t aSize1, size_t aSize2, jemalloc_stats_t& aStats)
> 
> I'd probably pass in large_max and chunksize directly, rather than via
> aStats.

I'm not sold on this. Making this argument, we should want the same for CanReallocInPlace, which we'd need 3 arguments for (1 more for the page size), which seems like too much for callers. And if we don't do it for CanReallocInPlace, then when do it for IsSameRoundedHugeClass?

Note that eventually, I'm leaning towards the test accessing the guts of mozjemalloc, so it will have access to things like SizeClass, and the various globals. Not quite there yet, though.
Comment on attachment 8929206 [details]
Bug 1418104 - Allow to pass parameters when creating arenas.

https://reviewboard.mozilla.org/r/200488/#review205670


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

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


::: memory/build/mozjemalloc.cpp:1108
(Diff revision 2)
>      mPrivateArenas.Init();
> +    arena_params_t params;
> +    // The main arena allows more dirty pages than the default for other arenas.
> +    params.mMaxDirty = opt_dirty_max;
>      mDefaultArena =
> -      mLock.Init() ? CreateArena(/* IsPrivate = */ false) : nullptr;
> +      mLock.Init() ? CreateArena(/* IsPrivate = */ false, &params) : nullptr;

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

      mLock.Init() ? CreateArena(/* IsPrivate = */ false, &params) : nullptr;
                                 ^
                                 /* aIsPrivate = */

::: memory/build/mozjemalloc.cpp:2222
(Diff revision 2)
>      // The arena will essentially be leaked if this function is
>      // called with `false`, but it doesn't matter at the moment.
>      // because in practice nothing actually calls this function
>      // with `false`, except maybe at shutdown.
> -    arena = gArenas.CreateArena(/* IsPrivate = */ false);
> +    arena =
> +      gArenas.CreateArena(/* IsPrivate = */ false, /* Params = */ nullptr);

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

      gArenas.CreateArena(/* IsPrivate = */ false, /* Params = */ nullptr);
                          ^
                          /* aIsPrivate = */

::: memory/build/mozjemalloc.cpp:2222
(Diff revision 2)
>      // The arena will essentially be leaked if this function is
>      // called with `false`, but it doesn't matter at the moment.
>      // because in practice nothing actually calls this function
>      // with `false`, except maybe at shutdown.
> -    arena = gArenas.CreateArena(/* IsPrivate = */ false);
> +    arena =
> +      gArenas.CreateArena(/* IsPrivate = */ false, /* Params = */ nullptr);

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

      gArenas.CreateArena(/* IsPrivate = */ false, /* Params = */ nullptr);
                                                   ^
                                                   /* aParams = */

::: memory/build/mozjemalloc.cpp:4689
(Diff revision 2)
>  template<>
>  inline arena_id_t
> -MozJemalloc::moz_create_arena()
> +MozJemalloc::moz_create_arena_with_params(arena_params_t* aParams)
>  {
>    if (malloc_init()) {
> -    arena_t* arena = gArenas.CreateArena(/* IsPrivate = */ true);
> +    arena_t* arena = gArenas.CreateArena(/* IsPrivate = */ true, aParams);

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

    arena_t* arena = gArenas.CreateArena(/* IsPrivate = */ true, aParams);
                                         ^
                                         /* aIsPrivate = */
Comment on attachment 8929206 [details]
Bug 1418104 - Allow to pass parameters when creating arenas.

https://reviewboard.mozilla.org/r/200488/#review205674


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

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


::: memory/build/mozjemalloc.cpp:1108
(Diff revision 3)
>      mPrivateArenas.Init();
> +    arena_params_t params;
> +    // The main arena allows more dirty pages than the default for other arenas.
> +    params.mMaxDirty = opt_dirty_max;
>      mDefaultArena =
> -      mLock.Init() ? CreateArena(/* IsPrivate = */ false) : nullptr;
> +      mLock.Init() ? CreateArena(/* IsPrivate = */ false, &params) : nullptr;

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

      mLock.Init() ? CreateArena(/* IsPrivate = */ false, &params) : nullptr;
                                 ^
                                 /* aIsPrivate = */

::: memory/build/mozjemalloc.cpp:2222
(Diff revision 3)
>      // The arena will essentially be leaked if this function is
>      // called with `false`, but it doesn't matter at the moment.
>      // because in practice nothing actually calls this function
>      // with `false`, except maybe at shutdown.
> -    arena = gArenas.CreateArena(/* IsPrivate = */ false);
> +    arena =
> +      gArenas.CreateArena(/* IsPrivate = */ false, /* Params = */ nullptr);

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

      gArenas.CreateArena(/* IsPrivate = */ false, /* Params = */ nullptr);
                          ^
                          /* aIsPrivate = */

::: memory/build/mozjemalloc.cpp:2222
(Diff revision 3)
>      // The arena will essentially be leaked if this function is
>      // called with `false`, but it doesn't matter at the moment.
>      // because in practice nothing actually calls this function
>      // with `false`, except maybe at shutdown.
> -    arena = gArenas.CreateArena(/* IsPrivate = */ false);
> +    arena =
> +      gArenas.CreateArena(/* IsPrivate = */ false, /* Params = */ nullptr);

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

      gArenas.CreateArena(/* IsPrivate = */ false, /* Params = */ nullptr);
                                                   ^
                                                   /* aParams = */

::: memory/build/mozjemalloc.cpp:4689
(Diff revision 3)
>  template<>
>  inline arena_id_t
> -MozJemalloc::moz_create_arena()
> +MozJemalloc::moz_create_arena_with_params(arena_params_t* aParams)
>  {
>    if (malloc_init()) {
> -    arena_t* arena = gArenas.CreateArena(/* IsPrivate = */ true);
> +    arena_t* arena = gArenas.CreateArena(/* IsPrivate = */ true, aParams);

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

    arena_t* arena = gArenas.CreateArena(/* IsPrivate = */ true, aParams);
                                         ^
                                         /* aIsPrivate = */
Comment on attachment 8929206 [details]
Bug 1418104 - Allow to pass parameters when creating arenas.

https://reviewboard.mozilla.org/r/200488/#review205680
Attachment #8929206 - Flags: review?(n.nethercote) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/5f8951b44feb
Initialize the allocator in jemalloc_stats. r=njn
https://hg.mozilla.org/integration/autoland/rev/646ad0d1720a
Allow to pass parameters when creating arenas. r=njn
https://hg.mozilla.org/integration/autoland/rev/72a526fe3d79
Add various realloc, junk and poisoning unit tests. r=njn
You need to log in before you can comment on or make changes to this bug.