Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
May 24, 2024
Merged

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

merged 9 commits into from
May 24, 2024

Conversation

nwf-msr
Copy link
Contributor

@nwf-msr nwf-msr commented Jan 4, 2024

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 nwf-msr requested review from mjp41 and davidchisnall January 4, 2024 21:06
Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I wonder if we should make STEP or SIZE configurable from CMake, so we can experiment with configurations.

It would be interesting to reduce step to 8, which would get us 16, 24, 32, 40, 48, 56, 64, 80, ...

@nwf-msr
Copy link
Contributor Author

nwf-msr commented Jan 5, 2024

Like that?

@mjp41
Copy link
Member

mjp41 commented Jan 5, 2024

Like that?

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

@nwf-msr
Copy link
Contributor Author

nwf-msr commented Jan 5, 2024

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

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nwf-msr
Copy link
Contributor Author

nwf-msr commented Jan 6, 2024

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.

@mjp41
Copy link
Member

mjp41 commented Jan 6, 2024

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

@mjp41
Copy link
Member

mjp41 commented Jan 8, 2024

@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
Copy link
Member

mjp41 commented Jan 9, 2024

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
Copy link
Member

mjp41 commented Jan 9, 2024

memory_nwf
time_nwf

nwf-msr added 6 commits May 11, 2024 02:42
The sizeclass was already testing most of this, so just add the missing bits.
Forgo some tests whose failure would have implied earlier failures.

This moves the last dynamic call of size_to_sizeclass_const into tests
(and so, too, to_exp_mant_const).  sizeclasstable.h still contains a static
call to compute NUM_SMALL_SIZECLASSES from MAX_SMALL_SIZECLASS_SIZE.
Only its _const sibling is used, and little at that, now that almost everything
to do with sizes and size classes is table-driven.
This just means I don't need to remember to set a breakpoint on exit
Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

src/snmalloc/mem/sizeclasstable.h Outdated Show resolved Hide resolved
src/snmalloc/mem/sizeclasstable.h Outdated Show resolved Hide resolved
@nwf-msr
Copy link
Contributor Author

nwf-msr commented May 24, 2024

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

@nwf-msr nwf-msr merged commit 846a926 into microsoft:main May 24, 2024
46 checks passed
@nwf-msr nwf-msr deleted the 202401-sizeclass-min-step-vs-alloc branch May 24, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants