snmalloc icon indicating copy to clipboard operation
snmalloc copied to clipboard

NFC: sizeclass: differentiate minimum step size and minimum allocation sizes

Open nwf-msr opened this issue 1 year ago • 8 comments

No functional change intended yet. Just going through the exercise of separating MIN_ALLOC_SIZE's two hats, but leaving the two at the same value.

So why?

We think this might be useful, letting us use less space in #637 (by leaving MIN_ALLOC_STEP_SIZE at 16 but raising MIN_ALLOC_SIZE to 32, meaning that sizeclasses 0 and 1 would not be allocable), but it might be more generally useful. It's also probably easier to review without all the rest of that mess at the same time. Speaking of review, as per usual, this might be best read in commit order rather than all at once, and the punchline is in the last commit.

nwf-msr avatar Jan 04 '24 21:01 nwf-msr

Like that?

nwf-msr avatar Jan 05 '24 17:01 nwf-msr

Like that?

Is there a static_assert that the main is at least two pointers?

mjp41 avatar Jan 05 '24 18:01 mjp41

Is there a static_assert that the main is at least two pointers?

Yes, two, sort of. Once directly by the configuration:

https://github.com/nwf-msr/snmalloc/blob/ab87e2b49b7371a1fa93a96f131703a87925fcd3/src/snmalloc/ds/allocconfig.h#L104-L106

And once by the freelist::Object that actually imposes the requirement:

https://github.com/nwf-msr/snmalloc/blob/ab87e2b49b7371a1fa93a96f131703a87925fcd3/src/snmalloc/mem/freelist.h#L429-L431

nwf-msr avatar Jan 05 '24 18:01 nwf-msr

Out of curiosity, I did a little parameter sweeping with mimalloc-bench. aNN is the minimum size, sNN is the step size. (That is, a16-s16 is the current policy on amd64.) Don't take these results too seriously, they're from my laptop and the machine was not particularly quiesced at the time.

image

It could be more interesting to do an analogous sweep on CHERI, where a32-s8 or a32-s16 might be of interest, or with #637, where a64-s16 or a64-s32 might be.

nwf-msr avatar Jan 06 '24 02:01 nwf-msr

Looks pretty noisy, but I can run some experiments on Monday

mjp41 avatar Jan 06 '24 08:01 mjp41

@nwf-msr would it make sense to add something like:

  if (SNMALLOC_BENCHMARK_INDIVIDUAL_CONFIGS)
    foreach (STEP 8 16 32)
      foreach (MIN 16 32 64)
        if ((${STEP} EQUAL 8) AND (${MIN} EQUAL 64))
          continue()
        endif()
        set(DEFINES "-DSNMALLOC_MIN_ALLOC_STEP_SIZE=${STEP}" "-DSNMALLOC_MIN_ALLOC_SIZE=${MIN}")
        set(NAME "step-${STEP}-min-${MIN}")
        add_shim(snmallocshim-${NAME} SHARED ${SHIM_FILES})
        target_compile_definitions(snmallocshim-${NAME} PRIVATE ${DEFINES})
        if (SNMALLOC_BUILD_TESTING)
          make_tests(${NAME} ${DEFINES})
        endif()
      endforeach()
    endforeach()
  endif() # SNMALLOC_BENCHMARK_INDIVIDUAL_CONFIGS

So that we can rebuild the libraries for the test?

mjp41 avatar Jan 08 '24 14:01 mjp41

Here is running on my perf VM in Azure:

memory_nwf time_nwf

I dropped a couple of the noisier ones, but overall it doesn't have much change.

mjp41 avatar Jan 09 '24 11:01 mjp41

memory_nwf time_nwf

mjp41 avatar Jan 09 '24 11:01 mjp41

Taking those two suggestions; thanks! Will merge as soon as CI's happy.

nwf-msr avatar May 24 '24 17:05 nwf-msr