Closed Bug 1414168 Opened 2 years ago Closed 2 years ago

Tweak run size calculation

Categories

(Core :: Memory Allocator, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(8 files)

For example, on 64-bits linux, arena_bin_run_size_calc currently chooses 8k runs for size classes between 96 and 176, but doing the math, except for 128 and 160, the same number of allocations could be handled with 2 runs of 4k.

The same can also be true for larger size classes: you can fit the same number of 192 bytes allocations in 3 4k runs than you can in one run of 12k (which is the size currently chosen).
Comment on attachment 8926263 [details]
Bug 1414168 - Demystify the last test in the main arena_bin_run_size_calc loop.

https://reviewboard.mozilla.org/r/197528/#review202746


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926264 [details]
Bug 1414168 - Change and move the relaxed calculation rule for small size classes.

https://reviewboard.mozilla.org/r/197530/#review202750


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926263 [details]
Bug 1414168 - Demystify the last test in the main arena_bin_run_size_calc loop.

https://reviewboard.mozilla.org/r/197528/#review202752


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926264 [details]
Bug 1414168 - Change and move the relaxed calculation rule for small size classes.

https://reviewboard.mozilla.org/r/197530/#review202758


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926267 [details]
Bug 1414168 - Change how run sizes are calculated.

https://reviewboard.mozilla.org/r/197536/#review202760


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926267 [details]
Bug 1414168 - Change how run sizes are calculated.

https://reviewboard.mozilla.org/r/197536/#review202762


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926263 [details]
Bug 1414168 - Demystify the last test in the main arena_bin_run_size_calc loop.

https://reviewboard.mozilla.org/r/197528/#review202766


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926264 [details]
Bug 1414168 - Change and move the relaxed calculation rule for small size classes.

https://reviewboard.mozilla.org/r/197530/#review202770


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926267 [details]
Bug 1414168 - Change how run sizes are calculated.

https://reviewboard.mozilla.org/r/197536/#review202772


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926274 [details]
Bug 1414168 - Move bin initialization to a method of the arena_bin_t class.

https://reviewboard.mozilla.org/r/197544/#review202776


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Blocks: 1415454
Comment on attachment 8926263 [details]
Bug 1414168 - Demystify the last test in the main arena_bin_run_size_calc loop.

https://reviewboard.mozilla.org/r/197528/#review202786


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926263 [details]
Bug 1414168 - Demystify the last test in the main arena_bin_run_size_calc loop.

https://reviewboard.mozilla.org/r/197528/#review202788


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926264 [details]
Bug 1414168 - Change and move the relaxed calculation rule for small size classes.

https://reviewboard.mozilla.org/r/197530/#review202790


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926264 [details]
Bug 1414168 - Change and move the relaxed calculation rule for small size classes.

https://reviewboard.mozilla.org/r/197530/#review202792


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926267 [details]
Bug 1414168 - Change how run sizes are calculated.

https://reviewboard.mozilla.org/r/197536/#review202796


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926267 [details]
Bug 1414168 - Change how run sizes are calculated.

https://reviewboard.mozilla.org/r/197536/#review202798


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926274 [details]
Bug 1414168 - Move bin initialization to a method of the arena_bin_t class.

https://reviewboard.mozilla.org/r/197544/#review202800


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926274 [details]
Bug 1414168 - Move bin initialization to a method of the arena_bin_t class.

https://reviewboard.mozilla.org/r/197544/#review202802


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926279 [details]
Bug 1414168 - Rename arena_run_t fields.

https://reviewboard.mozilla.org/r/197546/#review202804


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8926279 [details]
Bug 1414168 - Rename arena_run_t fields.

https://reviewboard.mozilla.org/r/197546/#review202806


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

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Mike, can you do before/after AWSY runs on this before landing? Something like this should do the trick:

./mach try -b o -p linux64,macosx64,win32,win64 -u awsy-e10s -t none --rebuild 5

I think the assumption is this might improve memory use / reduce fragmentation (heap-overhead/bin-unused), but it would be nice to test first.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8926262 [details]
Bug 1414168 - Split the condition for the main arena_bin_run_size_calc loop into pieces.

https://reviewboard.mozilla.org/r/197526/#review203030
Attachment #8926262 - Flags: review?(n.nethercote) → review+
Comment on attachment 8926265 [details]
Bug 1414168 - Avoid padding near the beginning of arena_run_t.

https://reviewboard.mozilla.org/r/197532/#review203040

::: memory/build/mozjemalloc.cpp:843
(Diff revision 1)
>  {
>  #if defined(MOZ_DIAGNOSTIC_ASSERT_ENABLED)
>    uint32_t magic;
>  #define ARENA_RUN_MAGIC 0x384adf93
> +
> +  // On 64-bits platforms, having the arena_bin_t pointer following

Nit: "64-bit"
Attachment #8926265 - Flags: review?(n.nethercote) → review+
Comment on attachment 8926263 [details]
Bug 1414168 - Demystify the last test in the main arena_bin_run_size_calc loop.

https://reviewboard.mozilla.org/r/197528/#review203032

r=me with the nit below addressed.

::: memory/build/mozjemalloc.cpp:3026
(Diff revision 1)
>      if (RUN_MAX_OVRHD * (bin->mSizeClass << 3) <= RUN_MAX_OVRHD_RELAX) {
>        break;
>      }
>  
> -    // Try to keep the run overhead below RUN_MAX_OVRHD.
> -    if ((try_reg0_offset << RUN_BFP) <= RUN_MAX_OVRHD * try_run_size) {
> +    // Try to keep the run overhead below 1.5%.
> +    if (Fraction(try_reg0_offset, try_run_size) <= 1.5_percent) {

All this is good, but the 1.5_percent value should be given a name and put into a #define or a constant. The two 1.5% mentions in the comments will also need updating.
Attachment #8926263 - Flags: review?(n.nethercote) → review+
Comment on attachment 8926264 [details]
Bug 1414168 - Change and move the relaxed calculation rule for small size classes.

https://reviewboard.mozilla.org/r/197530/#review203050
Attachment #8926264 - Flags: review?(n.nethercote) → review+
Comment on attachment 8926266 [details]
Bug 1414168 - Base run offset calculations on the fixed header size, excluding regs_mask.

https://reviewboard.mozilla.org/r/197534/#review203052

Nice.

::: commit-message-ef329:5
(Diff revision 1)
> +Bug 1414168 - Base run offset calculations on the raw header size, excluding regs_mask. r?njn
> +
> +On 64-bits platforms, sizeof(arena_run_t) includes a padding at the end
> +of the struct to align to 64-bits, since the last field, regs_mask, is
> +32-bits, and it's offset can be a multiple of 64-bits depending on the

Nit: "its"

::: memory/build/mozjemalloc.cpp:2966
(Diff revision 1)
>  {
>    size_t try_run_size, good_run_size;
>    unsigned good_nregs, good_mask_nelms, good_reg0_offset;
>    unsigned try_nregs, try_mask_nelms, try_reg0_offset;
> +  // Size of the run header, excluding regs_mask.
> +  static const size_t kRawHeaderSize = offsetof(arena_run_t, regs_mask);

"Raw" doesn't feel quite right. It's the size of the non-variable part of the struct. kFixedHeaderSize? kNonVariableHeaderSize? Not sure, you choose.
Attachment #8926266 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #38)
> "Raw" doesn't feel quite right. It's the size of the non-variable part of
> the struct. kFixedHeaderSize? kNonVariableHeaderSize? Not sure, you choose.

I like Fixed.

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #33)
> Mike, can you do before/after AWSY runs on this before landing? Something
> like this should do the trick:
> 
> ./mach try -b o -p linux64,macosx64,win32,win64 -u awsy-e10s -t none
> --rebuild 5
> 
> I think the assumption is this might improve memory use / reduce
> fragmentation (heap-overhead/bin-unused), but it would be nice to test first.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a64f536e170f&newProject=try&newRevision=c8f404da5758&framework=4
Flags: needinfo?(mh+mozilla)
Comment on attachment 8926267 [details]
Bug 1414168 - Change how run sizes are calculated.

https://reviewboard.mozilla.org/r/197536/#review203060

So there are two benefits here, right?
- (Minor) Seven size classes have reduced overhead, but only three have increase overhead.
- (Major) Lots of sizes classes have smaller runs, some of them much smaller, which reduces fragmentation.

::: commit-message-363cf:39
(Diff revision 3)
> +  2.4%.
> +
> +This leads to an uneven set of run sizes:
> + size class    before   after
> +   4            4 KiB    4 KiB
> +   8            4 KiB    4 KiB

I'd love to the the 1st and 3rd columns in a comment. (Possibly in a more compact form than this, i.e. with four runs per line.)

::: commit-message-363cf:73
(Diff revision 3)
> +   464         28 KiB   16 KiB
> +   480         28 KiB    8 KiB
> +   496         28 KiB   20 KiB
> +   512         32 KiB   32 KiB
> +   1024        64 KiB   64 KiB
> +   2048       129 KiB  128 KiB

129? Typo?

::: memory/build/mozjemalloc.cpp:2996
(Diff revision 3)
> +    // If the overhead is larger than the size class, it means the size class
> +    // is small and doesn't align very well with the header. It's desirable to
> +    // have smaller run sizes for them, so relax the overhead requirement to
> +    // 2.4%.
> +    if (try_reg0_offset > bin->mSizeClass) {
> +      if (Fraction(try_reg0_offset, try_run_size) <= 2.4_percent) {

You should have a name for the 2.4% constant, too.

::: memory/build/mozjemalloc.cpp:3025
(Diff revision 3)
>  
>    // Copy final settings.
> -  bin->mRunSize = good_run_size;
> -  bin->mRunNumRegions = good_nregs;
> -  bin->mRunNumRegionsMask = good_mask_nelms;
> -  bin->mRunFirstRegionOffset = good_reg0_offset;
> +  bin->mRunSize = try_run_size;
> +  bin->mRunNumRegions = try_nregs;
> +  bin->mRunNumRegionsMask = try_mask_nelms;
> +  bin->mRunFirstRegionOffset = try_reg0_offset;

The new loop structure is much better, and getting rid of the `good_` variables is good too.
Attachment #8926267 - Flags: review?(n.nethercote) → review+
Comment on attachment 8926274 [details]
Bug 1414168 - Move bin initialization to a method of the arena_bin_t class.

https://reviewboard.mozilla.org/r/197544/#review203062

::: memory/build/mozjemalloc.cpp:2967
(Diff revision 2)
> +
>    try_run_size = gPageSize;
>  
> +  mCurrentRun = nullptr;
> +  mNonFullRuns.Init();
> +  mSizeClass = aSizeClass.Size();

This is pre-existing, but `mSizeClass` isn't a great name for a field that doesn't have type `SizeClass`. `mSizeClassSize` is probably better, though it sounds a bit weird.
Attachment #8926274 - Flags: review?(n.nethercote) → review+
Comment on attachment 8926279 [details]
Bug 1414168 - Rename arena_run_t fields.

https://reviewboard.mozilla.org/r/197546/#review203064
Attachment #8926279 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #40)
> ::: commit-message-363cf:73
> (Diff revision 3)
> > +   464         28 KiB   16 KiB
> > +   480         28 KiB    8 KiB
> > +   496         28 KiB   20 KiB
> > +   512         32 KiB   32 KiB
> > +   1024        64 KiB   64 KiB
> > +   2048       129 KiB  128 KiB
> 
> 129? Typo?

Nope :) and that's why there's a overhead difference for that class.
(In reply to Mike Hommey [:glandium] from comment #43)
> (In reply to Nicholas Nethercote [:njn] from comment #40)
> > ::: commit-message-363cf:73
> > (Diff revision 3)
> > > +   464         28 KiB   16 KiB
> > > +   480         28 KiB    8 KiB
> > > +   496         28 KiB   20 KiB
> > > +   512         32 KiB   32 KiB
> > > +   1024        64 KiB   64 KiB
> > > +   2048       129 KiB  128 KiB
> > 
> > 129? Typo?
> 
> Nope :) and that's why there's a overhead difference for that class.

As per irc, it's actually 132. facepalm.

(In reply to Nicholas Nethercote [:njn] from comment #41)
> Comment on attachment 8926274 [details]
> Bug 1414168 - Move bin initialization to a method of the arena_bin_t class.
> 
> https://reviewboard.mozilla.org/r/197544/#review203062
> 
> ::: memory/build/mozjemalloc.cpp:2967
> (Diff revision 2)
> > +
> >    try_run_size = gPageSize;
> >  
> > +  mCurrentRun = nullptr;
> > +  mNonFullRuns.Init();
> > +  mSizeClass = aSizeClass.Size();
> 
> This is pre-existing, but `mSizeClass` isn't a great name for a field that
> doesn't have type `SizeClass`. `mSizeClassSize` is probably better, though
> it sounds a bit weird.

If you feel strongly about this, can you file a separate bug?
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/5ab2a7bd031f
Split the condition for the main arena_bin_run_size_calc loop into pieces. r=njn
https://hg.mozilla.org/integration/autoland/rev/bd35bf43d67d
Demystify the last test in the main arena_bin_run_size_calc loop. r=njn
https://hg.mozilla.org/integration/autoland/rev/076b6f406f2c
Change and move the relaxed calculation rule for small size classes. r=njn
https://hg.mozilla.org/integration/autoland/rev/ef39a9dff092
Avoid padding near the beginning of arena_run_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/29a122463ffc
Base run offset calculations on the fixed header size, excluding regs_mask. r=njn
https://hg.mozilla.org/integration/autoland/rev/11bc33786fd3
Change how run sizes are calculated. r=njn
https://hg.mozilla.org/integration/autoland/rev/182b840c8956
Move bin initialization to a method of the arena_bin_t class. r=njn
https://hg.mozilla.org/integration/autoland/rev/a4392df95024
Rename arena_run_t fields. r=njn
You need to log in before you can comment on or make changes to this bug.