[dbnode] Make index memory RSS usage flat block over block #2037
Conversation
|
Memory usage before on block rotations: Memory usage after: |
robskillington
changed the title
robskillington
changed the title
|
This will make tracking memory usage much easier since we don't have to rely on the kernel to evict pages under memory pressure - glad this will make operating M3DB simpler knowing how much real memory is wired! |
Codecov Report
@@ Coverage Diff @@
## master #2037 +/- ##
========================================
+ Coverage 71.6% 72.5% +0.8%
========================================
Files 1002 1009 +7
Lines 86381 86789 +408
========================================
+ Hits 61919 62941 +1022
+ Misses 20244 19622 -622
- Partials 4218 4226 +8
Continue to review full report at Codecov.
|
src/dbnode/persist/fs/index_read.go
Outdated
|
|
||
| // NB(bodu): Free mmaped bytes after we take the checksum so we don't get memory spikes at bootstrap time. | ||
| if err := mmap.Free(bytes); err != nil { | ||
| closeFiles() |
There was a problem hiding this comment.
nit: I know this is not your original code, but maybe best to use the following pattern that we do elsewhere to always call closeFiles() in case of error:
success := false
defer func() {
if success {
return // Returned successfully, do not need to close files
}
for _, file := range result.files { // This was originally closeFiles()
file.Close()
}
}()
// Rest of the code here
// Right before return successfully
success = true
return result, nilThere was a problem hiding this comment.
Then you don't need to call closeFiles() here explicitly and any new code paths automatically get the same behavior for free.
src/x/mmap/mmap_linux.go
Outdated
| // of waiting for memory pressure. | ||
| // NB(bodu): DO NOT FREE anonymously mapped memory or else it will null all of the underlying bytes as the | ||
| // memory is not file backed. | ||
| func Free(b []byte) error { |
There was a problem hiding this comment.
How do you feel about calling this DontNeed or something? Free makes me think looking through the code that the mmap should be unusable after that but thats not the case here
There was a problem hiding this comment.
It's definitely not 100% a "Free" operation, that's true. @notbdu perhaps we calls this MadviseDontNeed(..) to match the naming of Munmap(...)?
|
Does this P.R actually improve performance in any way, or is the goal to just not waste a bunch of O.S resources right after bootstrap on all these pages? Just seems a little weird to me since in theory the O.S should do an ok job of releasing these resources when they're needed. Is it to make the RSS of the m3db processes just look like its not using so much memory? |
|
@richardartoul this makes for easier tracking of memory that's actually in use. The go runtime itself also uses |
|
@notbdu cool was just curious |
|
Ran promremotebench w/ query activated for ~36h w/ no errors. |
What this PR does / why we need it:
Currently, memory usage scales w/ the size of the index up until retention. This is due to flushed index segment files being mmap'ed into memory and remaining in memory after we take a checksum. This PR naively frees file backed mmap'ed memory pages every Tick.
Special notes for your reviewer:
Left a
TODOfor implementing a more sophisticated free strategy in the future.Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?:
No.