Memory manager #221

rajdchak · 2025-02-10T00:27:49Z

Description of change

Memory Management for AAL library.

Relevant issues

Does this contribution introduce any breaking changes to the existing APIs or behaviors?

Does this contribution introduce any new public APIs or behaviors?

How was the contribution tested?

Does this contribution need a changelog entry?

I have updated the CHANGELOG or README if appropriate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

…ry to fix error java.lang.OutOfMemoryError: Java heap space

...stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/BlobStore.java

...integrationTest/java/software/amazon/s3/analyticsaccelerator/access/IntegrationTestBase.java

stubz151 · 2025-02-12T13:49:28Z

Can we please update the description this is a pretty big change and is hard to understand without a nice description.

...integrationTest/java/software/amazon/s3/analyticsaccelerator/access/IntegrationTestBase.java

...c/main/java/software/amazon/s3/analyticsaccelerator/io/physical/PhysicalIOConfiguration.java

stubz151 · 2025-02-12T13:33:42Z

...c/main/java/software/amazon/s3/analyticsaccelerator/io/physical/PhysicalIOConfiguration.java

      int metadataStoreCapacity,
      long blockSizeBytes,
      long readAheadBytes,
      long maxRangeSizeBytes,
      long partSizeBytes,
      double sequentialPrefetchBase,
      double sequentialPrefetchSpeed) {
-    Preconditions.checkArgument(blobStoreCapacity > 0, "`blobStoreCapacity` must be positive");
+    Preconditions.checkArgument(maxMemoryLimitAAL > 0, "`maxMemoryLimitAAL` must be positive");


I think we should agree on a limit here besides 0? Because if a user passes through 1 for example I think weird things will happen and I don't know if it will be obvious to the user what is happening. Can we not do something like 30 * blockSizeBytes or something

input-stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/Blob.java

...stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/BlobStore.java

...am/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/MemoryManager.java

...stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/BlobStore.java

ahmarsuhail · 2025-02-13T17:15:31Z

We discussed a bit in person. I am mostly ok with these changes for now! but will discuss with @fuatbasik how much work we want to do to make this reusable.

Currently I'm not happy with the BlockStore > Updates Memory Tracker > Asks BlobStore to clean up

We can have a contained cached interface, which we can reused in both the block store, the blob store, metadatastore which should make it easy to implement caching logic across our library. Just not sure how much of this we want to do now..

stubz151 · 2025-02-14T11:13:32Z

...stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/BlobStore.java

+
+  /** Checks memory usage of the BlobStore and schedules eviction if required */
+  private void checkBlobStoreMemoryUsageAndEvictIfRequired() {
+    if (shouldEvict() && isEvictionInProgress.compareAndSet(false, true)) {


that compareAndSet is cool, learnt something also I think is a good way of handling the cleanup. :D

stubz151 · 2025-02-14T11:17:23Z

...stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/BlobStore.java

-              }
-            });
+    this.blobMap = Collections.synchronizedMap(new LinkedHashMap<ObjectKey, Blob>(16, 0.75f, true));
+    if (configuration.getMaxMemoryLimitAAL() != Long.MAX_VALUE) {


could we move this to a method and make a comment about what it is doing? with all the numbers and if's it can be a bit confusing about what the end result looks like here.

+1, it is really hard to reason what is going on here.

rajdchak · 2025-02-17T12:47:08Z

...stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/BlobStore.java

+   * @return the max memory limit of the BlobStore
+   */
+  private long calculateDefaultMemoryLimit() {
+    return 3 * ONE_GB;


Decided this based on benchmark results.

fuatbasik

Thanks @rajdchak. I put few comments but need another pass. My biggest question is about ownership of concerns. It is quite confusing today if BlobStore should depend a memory manager of if a MemoryManager should depend on a BlobStore. Next, we have multiple counters on MemoryManager and BlobStore, one to trigger eviction another one to keep track of usage (i guess).

Another one is the role of a MemoryManager seems like implemented in the BlobStore but this is not extensible or maintainable. I will do another full pass once you address these.

fuatbasik · 2025-02-17T13:42:11Z

...ream/src/main/java/software/amazon/s3/analyticsaccelerator/S3SeekableInputStreamFactory.java

@@ -125,7 +128,12 @@ LogicalIO createLogicalIO(S3URI s3URI, StreamContext streamContext) throws IOExc
        return new ParquetLogicalIOImpl(
            s3URI,
            new PhysicalIOImpl(
-                s3URI, objectMetadataStore, objectBlobStore, telemetry, streamContext),
+                s3URI,


since we have 6 parameters shall we move to a builder here?

fuatbasik · 2025-02-17T13:42:58Z

...ream/src/main/java/software/amazon/s3/analyticsaccelerator/S3SeekableInputStreamFactory.java

@@ -134,7 +142,12 @@ LogicalIO createLogicalIO(S3URI s3URI, StreamContext streamContext) throws IOExc
        return new DefaultLogicalIOImpl(
            s3URI,
            new PhysicalIOImpl(
-                s3URI, objectMetadataStore, objectBlobStore, telemetry, streamContext),
+                s3URI,


lets avoid code duplication on creating PhysicalIOImpl

fuatbasik · 2025-02-17T13:50:02Z

...c/main/java/software/amazon/s3/analyticsaccelerator/io/physical/PhysicalIOConfiguration.java

-  /** Capacity, in blobs. {@link PhysicalIOConfiguration#DEFAULT_CAPACITY_BLOB_STORE} by default. */
-  @Builder.Default private int blobStoreCapacity = DEFAULT_CAPACITY_BLOB_STORE;
+  /**
+   * Max memory to be used by library. {@link PhysicalIOConfiguration#DEFAULT_MAX_MEMORY_LIMIT_AAL}


can you please add the unit here? Bytes, KBs? MBs?

it is even better if the name tells about the unit

fuatbasik · 2025-02-17T13:50:45Z

...c/main/java/software/amazon/s3/analyticsaccelerator/io/physical/PhysicalIOConfiguration.java

@@ -30,7 +30,7 @@
 @Builder
 @EqualsAndHashCode
 public class PhysicalIOConfiguration {
-  private static final int DEFAULT_CAPACITY_BLOB_STORE = 50;
+  private static final long DEFAULT_MAX_MEMORY_LIMIT_AAL = Long.MAX_VALUE;


nit: it would be lovely if the name also tells the unit. i.e. DEFAULT_MAX_MEMORY_BYTES

I am confused. Here we are setting default limit to Long.MAX_VALUE, later in the code, we are using Long.MAX_VALUE to confirm this value was not explicitly set and actually set a 3GB default number. Why not just set it to 3GB here?

fuatbasik · 2025-02-17T13:52:57Z

input-stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/Blob.java

+    try {
+      blockManager.makePositionAvailable(pos, ReadMode.SYNC);
+      return blockManager.getBlock(pos).get().read(pos);
+    } finally {


why we are comfortable with lack of a catch block?

I just wanted to decrement the active readers in any scenario. If there is any error thrown it can be handled by the caller methods, I don't think we want to catch anything here.

fuatbasik · 2025-02-17T14:31:39Z

...stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/BlobStore.java

+      LOG.info(
+          "Scheduling eviction because blobstore memory usage exceeded and currently is "
+              + memoryUsage.get());
+      evictionExecutor.execute(


why eviction business logic lives in BlobStore now that we have a MemoryManager?

fuatbasik · 2025-02-17T14:32:09Z

...stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/BlobStore.java

@@ -106,5 +253,6 @@ public int blobCount() {
  @Override
  public void close() {
    blobMap.forEach((k, v) -> v.close());
+    evictionExecutor.shutdownNow();


this should probably close the memorymanager right?

fuatbasik · 2025-02-17T14:39:00Z

...ream/src/main/java/software/amazon/s3/analyticsaccelerator/S3SeekableInputStreamFactory.java

@@ -75,6 +77,7 @@ public S3SeekableInputStreamFactory(
    this.objectFormatSelector = new ObjectFormatSelector(configuration.getLogicalIOConfiguration());
    this.objectBlobStore =
        new BlobStore(objectClient, telemetry, configuration.getPhysicalIOConfiguration());
+    this.memoryManager = new MemoryManager(objectBlobStore);


should a memoryManager have a blobStore or blobStore have a memorymanager?

fuatbasik · 2025-02-17T14:40:30Z

input-stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/Blob.java

+        nextPosition += bytesRead;
+      }
+      return numBytesRead;
+    } finally {


why try if not catching

I just wanted to decrement the active readers in any scenario so needed the finally block.

fuatbasik · 2025-02-17T14:57:39Z

...c/main/java/software/amazon/s3/analyticsaccelerator/io/physical/PhysicalIOConfiguration.java


-  private static final String BLOB_STORE_CAPACITY_KEY = "blobstore.capacity";
+  private static final String MAX_MEMORY_LIMIT_AAL_KEY = "max.memory";


nit: we can drop AAL from here. It is already part of the AAL.

rajdchak added 5 commits February 10, 2025 00:05

Memory manager

fa06c2c

Memory manager

c980005

Made some formatting changes and fixed tests

80baf27

Made memory manager non null in blockmanager

0bfdc81

Updated default blobstore max size to 20% of available java heap memo…

c923022

…ry to fix error java.lang.OutOfMemoryError: Java heap space

rajdchak commented Feb 10, 2025

View reviewed changes

...stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/data/BlobStore.java Outdated Show resolved Hide resolved

stubz151 reviewed Feb 10, 2025

View reviewed changes

...integrationTest/java/software/amazon/s3/analyticsaccelerator/access/IntegrationTestBase.java Show resolved Hide resolved

rajdchak added 2 commits February 10, 2025 15:48

Minor update to a comment

c380ae7

decrement readers after prefetch

3cdce88

stubz151 reviewed Feb 12, 2025

View reviewed changes

Addressed comments majorly making eviction async

ae82542

stubz151 reviewed Feb 14, 2025

View reviewed changes

rajdchak commented Feb 17, 2025

View reviewed changes

fuatbasik requested changes Feb 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory manager #221

Memory manager #221

rajdchak commented Feb 10, 2025 •

edited by ahmarsuhail

Loading

stubz151 commented Feb 12, 2025

stubz151 Feb 12, 2025

ahmarsuhail commented Feb 13, 2025

stubz151 Feb 14, 2025

stubz151 Feb 14, 2025

fuatbasik Feb 17, 2025

rajdchak Feb 17, 2025

fuatbasik left a comment

fuatbasik Feb 17, 2025

fuatbasik Feb 17, 2025

fuatbasik Feb 17, 2025

fuatbasik Feb 17, 2025

fuatbasik Feb 17, 2025

fuatbasik Feb 17, 2025

fuatbasik Feb 17, 2025

rajdchak Feb 17, 2025

fuatbasik Feb 17, 2025

fuatbasik Feb 17, 2025

fuatbasik Feb 17, 2025

fuatbasik Feb 17, 2025

rajdchak Feb 17, 2025

fuatbasik Feb 17, 2025


		private static final String BLOB_STORE_CAPACITY_KEY = "blobstore.capacity";
		private static final String MAX_MEMORY_LIMIT_AAL_KEY = "max.memory";

Memory manager #221

Are you sure you want to change the base?

Memory manager #221

Conversation

rajdchak commented Feb 10, 2025 • edited by ahmarsuhail Loading

Description of change

Relevant issues

Does this contribution introduce any breaking changes to the existing APIs or behaviors?

Does this contribution introduce any new public APIs or behaviors?

How was the contribution tested?

Does this contribution need a changelog entry?

stubz151 commented Feb 12, 2025

Choose a reason for hiding this comment

ahmarsuhail commented Feb 13, 2025

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

fuatbasik 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

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

rajdchak commented Feb 10, 2025 •

edited by ahmarsuhail

Loading