- Notifications
You must be signed in to change notification settings - Fork124
Fix build on aarch64 (e.g. Apple Silicon)#218
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Most architectures such as x86 and x86-64 use 4KiB, while aarch64 systems can use 4, 16 or 64KiB. Apple Silicon machines currently have 16KiB page sizes. The LMDB documentation states that map size should be a multiple of the page size, so a 100KiB default map size for tests fails for systems with a page size that is not 4 KiB.This commit simply increases the map size for tests. Of course, this applies only to these tests, as in production, users can choose the appropriate map size for their requirements and system architecture. It is not straightforward to determine architecture page size at runtime. Otherwise, it might be more appropriate to dynamically determine a minimum map size.
The options for determining architecture page size are a) environment variables, b) unsafe and undocumented JDK internals or c) looking to get answer using FFI. Given this test is asking checking the statistics returned by LMDB, on balance it is reasonable to simply validate we're getting a multiple of 4096 in the statistics report.
Uh oh!
There was an error while loading.Please reload this page.
Member
benalexau commentedApr 25, 2023
Thanks@wardle, much appreciated! |
ContributorAuthor
wardle commentedApr 25, 2023
It's the least I can do. Thank you for all your work! |
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
This pull request adopts an approach in which it is the library client that must consider appropriate map sizes. This is because map size is determined by the use case, and by system architecture. The documentation for LMDB clearly states that map size must be a multiple of the OS virtual memory page size. For most architectures this remains 4KiB, but newer architectures such as aarch64 have used 16KiB or 64KiB. Apple Silicon uses 16KiB.
As such, this pull request only changes tests so that the test suite passes on architectures with a larger page size than 4KiB.
The alternative is to help clients by massaging requested map sizes into something a multiple of the current architecture's page size, but this might be brittle. I would think it a reasonable assumption that consumers of lmdbjava will understand the issues here [although on reflection, perhaps the javadoc should make this very clear]. LMDB itself makes no attempt to fix map size based on its knowledge of system architecture and page size, and I think it reasonable that lmdbjava adopts that approach.
On MacBook Pro M1: