Closed
Bug 1418104
Opened 6 years ago
Closed 6 years ago
Add various realloc, junk and poisoning unit tests
Categories
(Core :: Memory Allocator, enhancement)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(3 files)
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
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, ¶ms); Warning: Argument name 'isprivate' in comment does not match parameter name 'aisprivate' [clang-tidy: misc-argument-comment] arena = gArenas.CreateArena(/* IsPrivate = */ false, ¶ms); ^ /* 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 = */
Comment 5•6 years ago
|
||
mozreview-review |
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 6•6 years ago
|
||
mozreview-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 7•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 8•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•6 years ago
|
||
mozreview-review |
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, ¶ms) : nullptr; Warning: Argument name 'isprivate' in comment does not match parameter name 'aisprivate' [clang-tidy: misc-argument-comment] mLock.Init() ? CreateArena(/* IsPrivate = */ false, ¶ms) : 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 14•6 years ago
|
||
mozreview-review |
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, ¶ms) : nullptr; Warning: Argument name 'isprivate' in comment does not match parameter name 'aisprivate' [clang-tidy: misc-argument-comment] mLock.Init() ? CreateArena(/* IsPrivate = */ false, ¶ms) : 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 15•6 years ago
|
||
mozreview-review |
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+
Comment 16•6 years ago
|
||
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
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5f8951b44feb https://hg.mozilla.org/mozilla-central/rev/646ad0d1720a https://hg.mozilla.org/mozilla-central/rev/72a526fe3d79
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•