Only commit memory on windows when initially claiming the frame (#4047)

kuzudb · Aug 8, 2024 · 83806ee · 83806ee
1 parent 13b20ef
commit 83806ee
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 18 deletions.
diff --git a/src/include/storage/buffer_manager/vm_region.h b/src/include/storage/buffer_manager/vm_region.h
@@ -29,13 +29,9 @@ class VMRegion {
     bool contains(const uint8_t* address) const {
         return address >= region && address < region + getMaxRegionSize();
     }
-#ifdef _WIN32
-    uint8_t* getFrame(common::frame_idx_t frameIdx);
-#else
     inline uint8_t* getFrame(common::frame_idx_t frameIdx) {
         return region + ((std::uint64_t)frameIdx * frameSize);
     }
-#endif
 
 private:
     inline uint64_t getMaxRegionSize() const {

diff --git a/src/storage/buffer_manager/buffer_manager.cpp b/src/storage/buffer_manager/buffer_manager.cpp
@@ -13,6 +13,8 @@
 #include <exception>
 
 #include <eh.h>
+#include <errhandlingapi.h>
+#include <memoryapi.h>
 #include <windows.h>
 #include <winnt.h>
 #endif
@@ -288,6 +290,18 @@ bool BufferManager::claimAFrame(BMFileHandle& fileHandle, page_idx_t pageIdx,
     if (!reserve(pageSizeToClaim)) {
         return false;
     }
+#ifdef _WIN32
+    // We need to commit memory explicitly on Windows.
+    // Committing in this context means reserving physical memory/page file space for a segment of
+    // virtual memory. On Linux/Unix this is automatic when you write to the memory address.
+    auto result =
+        VirtualAlloc(getFrame(fileHandle, pageIdx), pageSizeToClaim, MEM_COMMIT, PAGE_READWRITE);
+    if (result == NULL) {
+        throw BufferManagerException(
+            stringFormat("VirtualAlloc MEM_COMMIT failed with error code {}: {}.", GetLastError(),
+                std::system_category().message(GetLastError())));
+    }
+#endif
     cachePageIntoFrame(fileHandle, pageIdx, pageReadPolicy);
     return true;
 }
@@ -373,7 +387,8 @@ void BufferManager::updateFrameIfPageIsInFrameWithoutLock(file_idx_t fileIdx,
     const uint8_t* newPage, page_idx_t pageIdx) {
     KU_ASSERT(fileIdx < fileHandles.size());
     auto& fileHandle = *fileHandles[fileIdx];
-    if (fileHandle.getPageState(pageIdx)) {
+    auto state = fileHandle.getPageState(pageIdx);
+    if (state && state->getState() != PageState::EVICTED) {
         memcpy(getFrame(fileHandle, pageIdx), newPage, BufferPoolConstants::PAGE_4KB_SIZE);
     }
 }

diff --git a/src/storage/buffer_manager/vm_region.cpp b/src/storage/buffer_manager/vm_region.cpp
@@ -45,19 +45,6 @@ VMRegion::VMRegion(PageSizeClass pageSizeClass, uint64_t maxRegionSize) : numFra
 #endif
 }
 
-#ifdef _WIN32
-uint8_t* VMRegion::getFrame(frame_idx_t frameIdx) {
-    auto result = VirtualAlloc(region + ((std::uint64_t)frameIdx * frameSize), frameSize,
-        MEM_COMMIT, PAGE_READWRITE);
-    if (result == NULL) {
-        throw BufferManagerException(stringFormat(
-            "VirtualAlloc MEM_COMMIT failed with error code {}: {}.", getMaxRegionSize(),
-            GetLastError(), std::system_category().message(GetLastError())));
-    }
-    return region + ((std::uint64_t)frameIdx * frameSize);
-}
-#endif
-
 VMRegion::~VMRegion() {
 #ifdef _WIN32
     VirtualFree(region, 0, MEM_RELEASE);