-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(sql): disk spill support for FastMap #3877
base: master
Are you sure you want to change the base?
Conversation
@puzpuzpuz Hi Andrei, I am trying to switch implementation to use It currently still has memory leak since I do not check for
looking forward to your feedback. *update: |
cccc77d
to
9ba338e
Compare
I'd avoid modifying
That's also fine - as a user, I wouldn't be happy if the |
@puzpuzpuz could you take a quick look on current PR state?
I would like your suggestion on the followings:
In addition, I found several parts where (to me, CMIIW) looks inconsistent. I marked them with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall direction looks good. Left a few comments here and there.
core/src/main/java/io/questdb/cairo/vm/MemoryCARWSpillable.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cairo/vm/MemoryCARWSpillable.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cairo/vm/MemoryCARWSpillable.java
Outdated
Show resolved
Hide resolved
|
||
import static org.junit.Assert.*; | ||
|
||
public class MemoryCARWSpillableTest extends AbstractCairoTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be some tests that assert the disk spill case (the file gets created when the memory grows and the memory contents are retained, the file gets deleted when the memory shrinks and so on). Also, some negative case tests would be great, e.g. if file creation fails, a meaningful exception is thrown; if file deletion fails on map shrink, an exception is thrown (yet, FastMap
may ignore it in this scenario as we need to delete the temp dir on startup).
core/src/main/java/io/questdb/cairo/vm/MemoryCARWSpillable.java
Outdated
Show resolved
Hide resolved
private String tmpFile; | ||
|
||
// TODO: For now assume the same pageSize | ||
public MemoryCARWSpillable(long pageSize, int maxPages, long diskSpillThreshold, CairoConfiguration cairoConf){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to have so many constructors. It's enough to keep only the one needed by the FastMap
. The same applies to all methods here - they can be FastMap
-specific and there is no need to keep them if they're unused. In fact, this call doesn't even have to implement MemoryCARW
especially considering that certain methods lose their meaning due to different underlying memories, e.g. getExtendSegmentSize()
, getPageCount()
or getPageSize()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see..
Tbh, I found several inconsistencies inside MemoryCMARWImpl and MemoryCARWImpl where it might refer to different things (but most of the methods there such as getExtendSegmentSize()
are derived from shared interface, so I believe they should convey the same meaning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, how should I set the pageSize for each memory type (filebased and plain mem)?
Is it configurable?
if so, what should be sensible default? 4kb?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the "normal" (non-file-baked) memory, the page size should be the same as FastMap
's initial size. For the mmapped memory, page size doesn't matter - the extend segment size does. I'd make both of these configurable, so that FastMap
passes them when initializing the heap object. Also, the extend segment size should be configurable through the CairoConfiguration
. The default value may be something like 16MB since we know that disk spill has to deal with large files and frequent file extensions will be costly with 4KB.
core/src/main/java/io/questdb/cairo/vm/MemoryCARWSpillable.java
Outdated
Show resolved
Hide resolved
Hi Andrei @puzpuzpuz. My apology for late follow-up 🙇 I updated in several places. Here is highlight from the last time:
Left to do:
I would like to hear your opinion, especially about the current config and settings. Thanks a lot for your time. P.S. |
return; | ||
} | ||
|
||
// MemoryCMARWImpl does not have memory breached checking, so adding one here. In addition, default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The growth policy doesn't have to be consistent in both spilled and non-spilled modes: in the in-memory mode it should grow as a power of two (just like it does currently in the master
branch), while in file-based mode it should grow in multiples of extendSegmentSize
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Andrei,
Sorry for late follow up.
Id like to clarify more detail about the extend behavior since Ive been overthinking about this. To start, this is my current understanding:
pageSize
should be power of two- in the in-mem mode, it grows as a power of two.
extendSegmentSize
should also be power of two.- when it grows, it grows as a multiple of extendSegmentSize (thus the size of mmapped heap is not necessarily power of two)
Then,
- In FastMap, the
MAX_HEAP_SIZE
is defined as(Integer.toUnsignedLong(-1) - 1) << 3
. However, it is not a power of 2 (which might also not an exact multiples ofextendSegmentSize
). How do you think the extend behavior should be when extending up toMAX_HEAP_SIZE
?
Is it okay to allow memory to extend beyond the MAX_HEAP_SIZE? And if it is allowed, is special handling required so that only part of the allocated memory can be used instead of all (in this case, we only use the memory up to MAX_HEAP_SIZE rather than its actual allocated size)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, lets say the requested size is n * pageSize
, what should be the allocated size?
In current implementation, CARW and CMARW has different implementation.
- CARW: requested
n * pageSize
-> givenn * pageSize
- CMARW: requested
n * pageSize
-> given(n+1) * pageSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- However, it is not a power of 2 (which might also not an exact multiples of
extendSegmentSize
). How do you think the extend behavior should be when extending up toMAX_HEAP_SIZE
?
MAX_HEAP_SIZE
is the max possible capacity of the heap, i.e. it's a threshold, so it doesn't have to be a power of 2. We use it to check if the heap capacity after growth would exceed the threshold, not to initialize the heap.
- Is it okay to allow memory to extend beyond the MAX_HEAP_SIZE?
No, this shouldn't be allowed considering that we won't be able to index entries beyond MAX_HEAP_SIZE
. It's better to fail fast when we detect such situation when growing the heap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, lets say the requested size is
n * pageSize
, what should be the allocated size?
In file-based mode, we should grow the file in pages (page size should be multiple of the OS page, see Files.PAGE_SIZE
). In in-memory mode, we should keep existing logic with the power of 2. In both cases, the allocated size should be equal or larger than the requested size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. So, lets say the extendSegmentSize
is set to 4GB (default value you proposed earlier) and the current heap size is 28GB (7 pages). When map is required to grow to 29GB, for example, since it extends based on page size, it will never be allowed since the next target size is 32GB = 34,359,738,368, while the max allowed heap is Integer.toUnsignedLong(-1) - 1) << 3 = 34,359,738,352
.
In that case, is it okay to just throw error, or should it be allowed to grow to the MAX_HEAP_SIZE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, is it okay to just throw error, or should it be allowed to grow to the MAX_HEAP_SIZE?
I'd say that if the requested size (29GB) is under MAX_HEAP_SIZE
, we should grow to MAX_HEAP_SIZE
. After that, the next growth attempt should fail.
So, lets say the
extendSegmentSize
is set to 4GB (default value you proposed earlier)
I recall that I mentioned 4GB as the disk spill threshold, not the extendSegmentSize
. I'd make extendSegmentSize
something in the 256-512MB range by default. The idea is to do less costly file growth operations, while not growing it too rapidly to avoid using the disk space too eagerly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall that I mentioned 4GB as the disk spill threshold, not the extendSegmentSize. I'd make extendSegmentSize something in the 256-512MB range by default. The idea is to do less costly file growth operations, while not growing it too rapidly to avoid using the disk space too eagerly.
I see
I'd say that if the requested size (29GB) is under MAX_HEAP_SIZE, we should grow to MAX_HEAP_SIZE. After that, the next growth attempt should fail.
In summary, does that mean the last extension may extend without respecting the extendSegmentSize
?
Or in other word, should we extend up to MAX_HEAP_SIZE, or up to the size below MAX_HEAP_SIZE which is multiple of extendSegmentSize
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In summary, does that mean the last extension may extend without respecting the
extendSegmentSize
?
Or in other word, should we extend up to MAX_HEAP_SIZE, or up to the size below MAX_HEAP_SIZE which is multiple ofextendSegmentSize
?
Yes, that sounds logical to me. File extension should try to use as much heap space as possible.
|
||
public class SpillableHeapMemoryTest extends AbstractCairoTest { | ||
private static FilesFacade ff = new FilesFacadeImpl(); | ||
private static Path spillPath = Path.getThreadLocal(root).concat("fastmap.tmp").$(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thread-local paths should be used in-place, without storing them in a field. Otherwise, chances are high that they'll get mutated or released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it also applies similarly when passing it down to class instantiation as an argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the class constructor doesn't store the path as a field, but uses it locally, that's fine. We have @Transient
annotation to mark arguments that shouldn't outlive the constructor call.
import static org.junit.Assert.*; | ||
|
||
public class SpillableHeapMemoryTest extends AbstractCairoTest { | ||
private static FilesFacade ff = new FilesFacadeImpl(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a good idea to add some tests around file errors, e.g. what happens when a disk spill fails since there is no disk space and so on. Check other tests that define an anonymous sub-class for TestFilesFacadeImpl
.
Left a few comments, including the one on the extension behavior. |
@puzpuzpuz Thanks a lot for taking your time to give a thorough review. That is really helpful for me. Will get back to you soon. |
Yes, there were some recent changes around |
WIP for #3848
Not ready for review, but created PR early for feedback