feat(sql): disk spill support for FastMap #3877

hilmialf · 2023-10-20T11:31:04Z

WIP for #3848
Not ready for review, but created PR early for feedback

CLAassistant · 2023-10-20T11:31:11Z

All committers have signed the CLA.

hilmialf · 2023-10-20T11:38:23Z

@puzpuzpuz Hi Andrei, I am trying to switch implementation to use MemoryCARWImpl first before adding mechanism to switch to mmap version.

It currently still has memory leak since I do not check for maxResize and MAX_HEAP_SIZE limit yet.
If the current PR state is in the good direction, I'll proceed as follows:

Add additional private vars inside MemoryCARWImpl (nResize, MAX_HEAP_SIZE) and perform check before each resize to obey the new limitation.
Continue the work on implementing the mmap part.

looking forward to your feedback.

*update:
I tried imposing limit (nResize, MAX_HEAP_SIZE) but MemoryCARWImpl does not seem to extend the same way as original FastMap impl (<<1 for every extend).

puzpuzpuz · 2023-10-20T16:41:59Z

Add additional private vars inside MemoryCARWImpl (nResize, MAX_HEAP_SIZE) and perform check before each resize to obey the new limitation.

I'd avoid modifying MemoryCARWImpl - this class already has sufficient general-purpose API. The checks can be made externally, i.e. in FastMap.

but MemoryCARWImpl does not seem to extend the same way as original FastMap impl (<<1 for every extend).

That's also fine - as a user, I wouldn't be happy if the FastMap file grows from 4GB to 8GB in one go while it really needs 5GB. FastMap should employ different growth strategy based on the heap type (file-baked or not).

hilmialf · 2023-10-22T03:32:31Z

@puzpuzpuz could you take a quick look on current PR state?
Here is what's changed:

pageSize is renamed to heapSize inside FastMap to make the name clearer.
created MemoryCARWSpillable along with unit tests. *let me revisit for the thoroughness, right now I just copied directly from MemoryCMARImplTest.

I would like your suggestion on the followings:

what should be the best default pageSize for CARWImpl inside MemoryCARWSpillable
similarly, what should be the best default pageSize for CMARWImpl inside MemoryCARWSpillable
Also, I wonder whether we need to make enforcement if both pageSize should be equal or not

In addition, I found several parts where (to me, CMIIW) looks inconsistent. I marked them with // TODO.
for instance:

extendedSegmentSize definition.

puzpuzpuz

The overall direction looks good. Left a few comments here and there.

core/src/main/java/io/questdb/cairo/vm/MemoryCARWSpillable.java

core/src/main/java/io/questdb/cairo/vm/api/MemoryCARW.java

puzpuzpuz · 2023-10-23T06:45:57Z

core/src/test/java/io/questdb/test/cairo/vm/MemoryCARWSpillableTest.java

+
+import static org.junit.Assert.*;
+
+public class MemoryCARWSpillableTest extends AbstractCairoTest {


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

puzpuzpuz · 2023-10-23T06:51:51Z

core/src/main/java/io/questdb/cairo/vm/MemoryCARWSpillable.java

+    private String tmpFile;
+
+    // TODO: For now assume the same pageSize
+    public MemoryCARWSpillable(long pageSize, int maxPages, long diskSpillThreshold, CairoConfiguration cairoConf){


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().

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)

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?

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

hilmialf · 2023-11-11T13:42:37Z

Hi Andrei @puzpuzpuz. My apology for late follow-up 🙇

I updated in several places. Here is highlight from the last time:

Added necessary Configuration
Updated places where FastMap is instantiated to use the new constructor.

Left to do:

Add test cases for spillable FastMap. Notes:

I did extensive testcase for SpillableHeapMemory in SpillableHeapMemoryTest. However, I have not done test yet from FastMapTest. Instead, I keep the current FastMapTest test case as it is with old FastMap constructor (unspillable memory).

I would like to hear your opinion, especially about the current config and settings.
In addition, I found the behavior of extend() for MemoryCARWImpl and MemoryCMARWImpl are different internally when the new size is multiple of pageSize. In current implementation, I somehow modify them so that they behave similarly. You can find the detail on the SpillableHeapMemory checkAndExtend() part. Please let me know if this is not what you thought it should be.

Thanks a lot for your time.

P.S.
Oops my formatter is unbehaving.. its somehow restructuring in unnecessary part of the code. Could you point out where I setup wrong with the formatter? I tried to follow the development guide.

puzpuzpuz · 2023-11-13T09:51:07Z

Oops my formatter is unbehaving.. its somehow restructuring in unnecessary part of the code. Could you point out where I setup wrong with the formatter? I tried to follow the development guide.

If you're using IntelliJ IDEA, it should pick up the code style automatically and it's checked in the repo. Personally I use the following actions on save, so that the edited files are always auto-formatted.

core/src/main/java/io/questdb/PropServerConfiguration.java

core/src/main/java/io/questdb/cairo/map/SpillableHeapMemory.java

puzpuzpuz · 2023-11-13T10:37:10Z

core/src/main/java/io/questdb/cairo/map/SpillableHeapMemory.java

+            return;
+        }
+
+        // MemoryCMARWImpl does not have memory breached checking, so adding one here. In addition, default


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.

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 of extendSegmentSize). How do you think the extend behavior should be when extending up to MAX_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)

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 -> given n * pageSize

CMARW: requested n * pageSize -> given (n+1) * pageSize

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 to MAX_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.

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.

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?

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.

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?

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?

Yes, that sounds logical to me. File extension should try to use as much heap space as possible.

puzpuzpuz · 2023-11-13T10:38:49Z

core/src/test/java/io/questdb/test/cairo/map/SpillableHeapMemoryTest.java

+
+public class SpillableHeapMemoryTest extends AbstractCairoTest {
+    private static FilesFacade ff = new FilesFacadeImpl();
+    private static Path spillPath = Path.getThreadLocal(root).concat("fastmap.tmp").$();


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.

I believe it also applies similarly when passing it down to class instantiation as an argument?

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.

puzpuzpuz · 2023-11-13T10:40:56Z

core/src/test/java/io/questdb/test/cairo/map/SpillableHeapMemoryTest.java

+import static org.junit.Assert.*;
+
+public class SpillableHeapMemoryTest extends AbstractCairoTest {
+    private static FilesFacade ff = new FilesFacadeImpl();


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.

puzpuzpuz · 2023-11-13T11:12:15Z

In addition, I found the behavior of extend() for MemoryCARWImpl and MemoryCMARWImpl are different internally when the new size is multiple of pageSize. In current implementation, I somehow modify them so that they behave similarly. You can find the detail on the SpillableHeapMemory checkAndExtend() part. Please let me know if this is not what you thought it should be.

Left a few comments, including the one on the extension behavior.

hilmialf · 2023-11-13T11:29:45Z

@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.
Additionally, I notice that the memory layout for FastMap has changed. Should I incorporate those changes to this PR?

puzpuzpuz · 2023-11-13T11:40:27Z

Additionally, I notice that the memory layout for FastMap has changed. Should I incorporate those changes to this PR?

Yes, there were some recent changes around FastMap, sorry for that. I'm afraid you'd have to resolve conflicts. Please let me know if you need any guidance with that.

hilmialf force-pushed the master branch 2 times, most recently from cccc77d to 9ba338e Compare October 20, 2023 13:41

Switch to use MemoryCARWImpl

0c19ea3

hilmialf force-pushed the master branch from 9ba338e to 0c19ea3 Compare October 20, 2023 13:42

Try imposing maxResize & MAX_HEAP_SIZE limit

e74f99a

hilmialf added 3 commits October 21, 2023 07:42

bugfix wrong initial appendOffset

aa588e5

bugfix wrong putbin implementation

f533267

Implement auto spill with threshold

170e026

puzpuzpuz reviewed Oct 23, 2023

View reviewed changes

hilmialf added 2 commits November 11, 2023 22:29

Implement SpillableHeapMemory and add default conf

63a28dc

Update FastMap instantiation in several places

85f9c7e

Revert unnecessary formatting

e63890e

puzpuzpuz added the New feature Feature requests label Nov 13, 2023

hilmialf commented Nov 13, 2023

View reviewed changes

core/src/main/java/io/questdb/PropServerConfiguration.java Show resolved Hide resolved

puzpuzpuz reviewed Nov 13, 2023

View reviewed changes

puzpuzpuz changed the title ~~disk spill support for FastMap~~ feat(sql): disk spill support for FastMap Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sql): disk spill support for FastMap #3877

feat(sql): disk spill support for FastMap #3877

hilmialf commented Oct 20, 2023 •

edited

CLAassistant commented Oct 20, 2023 •

edited

hilmialf commented Oct 20, 2023 •

edited

puzpuzpuz commented Oct 20, 2023

hilmialf commented Oct 22, 2023

puzpuzpuz left a comment

puzpuzpuz Oct 23, 2023

puzpuzpuz Oct 23, 2023

hilmialf Oct 23, 2023

hilmialf Oct 23, 2023

puzpuzpuz Oct 23, 2023

hilmialf commented Nov 11, 2023

puzpuzpuz commented Nov 13, 2023

puzpuzpuz Nov 13, 2023

hilmialf Nov 25, 2023

hilmialf Nov 25, 2023 •

edited

puzpuzpuz Nov 27, 2023

puzpuzpuz Nov 27, 2023

hilmialf Nov 28, 2023

puzpuzpuz Nov 28, 2023

hilmialf Nov 29, 2023

puzpuzpuz Nov 29, 2023

puzpuzpuz Nov 13, 2023

hilmialf Nov 25, 2023

puzpuzpuz Nov 27, 2023

puzpuzpuz Nov 13, 2023

puzpuzpuz commented Nov 13, 2023

hilmialf commented Nov 13, 2023

puzpuzpuz commented Nov 13, 2023


		import static org.junit.Assert.*;

		public class MemoryCARWSpillableTest extends AbstractCairoTest {

feat(sql): disk spill support for FastMap #3877

Are you sure you want to change the base?

feat(sql): disk spill support for FastMap #3877

Conversation

hilmialf commented Oct 20, 2023 • edited

CLAassistant commented Oct 20, 2023 • edited

hilmialf commented Oct 20, 2023 • edited

puzpuzpuz commented Oct 20, 2023

hilmialf commented Oct 22, 2023

puzpuzpuz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hilmialf commented Nov 11, 2023

puzpuzpuz commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hilmialf Nov 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puzpuzpuz commented Nov 13, 2023

hilmialf commented Nov 13, 2023

puzpuzpuz commented Nov 13, 2023

hilmialf commented Oct 20, 2023 •

edited

CLAassistant commented Oct 20, 2023 •

edited

hilmialf commented Oct 20, 2023 •

edited

hilmialf Nov 25, 2023 •

edited