Skip to content

Conversation

mjp41
Copy link
Member

@mjp41 mjp41 commented Mar 23, 2022

This is the first commits for adding the DecayRange. It involves

  • removing the redundant ChunkAllocator
  • Adding a field representing if a Range is concurrency safe
  • Fixing OE Stats
  • Improving the message printing.

@mjp41 mjp41 mentioned this pull request Mar 23, 2022
2 tasks
@mjp41 mjp41 force-pushed the predecayrange branch 4 times, most recently from 2dde429 to 0f38f82 Compare March 23, 2022 20:07
@mjp41 mjp41 requested a review from nwf March 23, 2022 20:24
Copy link
Contributor

@nwf nwf left a comment

Choose a reason for hiding this comment

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

Some minor nits; generally looks good.

Do you want to add a static_asserts in backend.h that GlobalRange is ConcurrencySafe?

@@ -282,7 +281,7 @@ namespace snmalloc
bumpptr = slab_end;
}

ChunkRecord* clear_slab(Metaslab* meta, smallsizeclass_t sizeclass)
Metaslab* clear_slab(Metaslab* meta, smallsizeclass_t sizeclass)
Copy link
Contributor

Choose a reason for hiding this comment

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

The return here was always just a type pun and can probably be done away with now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I had removed the use-site, but not the return.

}
return tid;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally approve of the idea, but should this actually be in the PAL so we can actually get the platform notion of a thread ID (when it's sufficiently inexpensive to do so)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly, I just needed some logging that I could uninterleave ;-) Happy for something better to be done. I can drop this commit from the PR, and someone can do something better, or I can leave this, and then someone can improve if they have bandwidth?

My preference is for the later, but what do you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a strict improvement, so no reason to drop it. Was mostly asking in case there was reason not to do the PAL thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to trace a bug in DecayRange which only exhibited with concurrency, and this was the quickest thing I could do to get a trace I could understand. I think we can engineer something better.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can engineer something better.

But not tonight ;-)

@@ -24,6 +24,8 @@ namespace snmalloc

static constexpr bool Aligned = pal_supports<AlignedAllocation, PAL>;

static constexpr bool ConcurrencySafe = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

While true at the moment, do we want to promise that all PALs have concurrency-safe (de)alloc routines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I could say false instead? Or we could add a feature flag to each Pal that says if it is true?

Copy link
Member Author

Choose a reason for hiding this comment

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

The Pals have always been assumed to be concurrency safe so far.

Copy link
Contributor

@nwf nwf 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'll leave it to you to squash into one or many commits.

mjp41 added 4 commits March 23, 2022 22:06
Automatically prepend messages with a thread id.  Makes debugging
easier.
Now we have an allocation free formatting routine, remove std::cout
from tracing.
Ranges can be safe to call from multiple threads.  This adds a constexpr
field to signify if that is the case.
@mjp41 mjp41 merged commit e77b9e2 into microsoft:main Mar 24, 2022
@mjp41 mjp41 deleted the predecayrange branch March 24, 2022 08:01
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