Skip to content

Conversation

@camelid
Copy link
Contributor

@camelid camelid commented Nov 19, 2023

This is part of adding Apple Silicon support to Mesh.

class LeaHeap2 :
public
Threshold<4096,
Threshold<4096, // FIXME: is this page-size dependent?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bpowers there were a couple of parts where I wasn't sure if the number was referring to the page size or another value, like here and below. I'm guessing this one should be changed to the page size (though not sure about the others), but do you know for sure?

Copy link
Owner

Choose a reason for hiding this comment

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

This particular heap layer is legacy code; I'm not sure it's being used by anyone. IIRC the size here was chosen arbitrarily (by me) but it might make sense to parameterize it by the system page size.

// macOS on Apple Silicon aligns 16K pages to a 16K boundary.
// FIXME: is this the correct alignment?
enum { Size = 16 * 1024UL };
enum { Alignment = 16 * 1024UL };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to find any documentation about the alignment for Apple Silicon pages. I'm assuming they're 16K aligned?

Copy link
Owner

Choose a reason for hiding this comment

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

I believe this is correct. I'm not sure if vm_page_size is constexpr but if so, we could use that instead of hardcoding the numbers. Anyway, probably should also put in an assertion (if one is not there already) in the call to mmap to verify alignment and size.

// macOS on Apple Silicon aligns 16K pages to a 16K boundary.
// FIXME: is this the correct alignment?
enum { Size = 16 * 1024UL };
enum { Alignment = 16 * 1024UL };
Copy link
Owner

Choose a reason for hiding this comment

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

I believe this is correct. I'm not sure if vm_page_size is constexpr but if so, we could use that instead of hardcoding the numbers. Anyway, probably should also put in an assertion (if one is not there already) in the call to mmap to verify alignment and size.

class LeaHeap2 :
public
Threshold<4096,
Threshold<4096, // FIXME: is this page-size dependent?
Copy link
Owner

Choose a reason for hiding this comment

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

This particular heap layer is legacy code; I'm not sure it's being used by anyone. IIRC the size here was chosen arbitrarily (by me) but it might make sense to parameterize it by the system page size.

@emeryberger emeryberger merged commit f97c110 into emeryberger:master May 12, 2024
@camelid camelid deleted the arm64 branch May 14, 2024 21:41
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