Make MemorySize a multiple of page size and not necessarily a power of two #233
Conversation
This file contains 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
TedHartMS
reviewed
Apr 11, 2024
| if (settings.MemorySizeBits != 0 && (settings.MemorySizeBits < LogSettings.kMinMemorySizeBits || settings.MemorySizeBits > LogSettings.kMaxMemorySizeBits)) | ||
| throw new TsavoriteException($"{nameof(settings.MemorySizeBits)} must be between {LogSettings.kMinMemorySizeBits} and {LogSettings.kMaxMemorySizeBits}, or may be 0 for ReadOnly TsavoriteLog"); | ||
| if (settings.MemorySizePages != 0 && (settings.MemorySizePages < LogSettings.kMinMemorySizePages || settings.MemorySizePages > LogSettings.kMaxMemorySizePages)) | ||
| throw new TsavoriteException($"{nameof(settings.MemorySizePages)} must be between {LogSettings.kMinMemorySizePages} and {LogSettings.kMaxMemorySizePages}, or may be 0 for ReadOnly TsavoriteLog"); | ||
| if (settings.MutableFraction < 0.0 || settings.MutableFraction > 1.0) |
There was a problem hiding this comment.
See comment in LogSetting regarding kMaxMemorySizePages
| /// <summary>Minimum number of pages for the size of the in-memory portion of the log</summary> | ||
| public const int kMinMemorySizePages = 1; // At least one page | ||
| /// <summary>Maximum number of pages for the size of the in-memory portion of the log</summary> | ||
| public const int kMaxMemorySizePages = int.MaxValue; | ||
|
|
There was a problem hiding this comment.
For this to reach the old kMaxMemorySizeBits equivalent, the PageSizeBits would have to be 31. Instead, I think it's better to keep a kMemorySizeMax = 1 << 62, then verify in AllocatorBase that the specified config does not exceed this
| @@ -204,7 +204,7 @@ internal LogSettings GetLogSettings() | |||
| LogDevice = LogDevice, | |||
| PageSizeBits = Utility.NumBitsPreviousPowerOf2(PageSize), | |||
| SegmentSizeBits = Utility.NumBitsPreviousPowerOf2(SegmentSize), | |||
| MemorySizeBits = ReadOnlyMode ? 0 : Utility.NumBitsPreviousPowerOf2(MemorySize), | |||
| MemorySizePages = ReadOnlyMode ? 0 : (int)(MemorySize / (1L << Utility.NumBitsPreviousPowerOf2(PageSize))), | |||
| ReadCopyOptions = ReadCopyOptions.None, | |||
There was a problem hiding this comment.
Use PageSizeBits instead of repeating Utility.NumBitsPreviousPowerOf2
added 2 commits
April 10, 2024 19:53
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
MemorySize is currently enforced to be a power of two. If memory is configured to be not a power of two, then the size is adjusted dynamically using EmptyPageCount. Also, the EmptyPageCount is not enforced during recovery.
This change removes the power of two constraint for MemorySize. With this, the memory size is constrained to be a multiple of PageSize. This makes MemorySize to be constant during recovery as well as regular operation.
The EmptyPageCount would still be used to control Object Store GC heap memory size.