[GitHub] [flink] azagrebin commented on a change in pull request #9693: [FLINK-13984] Separate on-heap and off-heap managed memory pools

[GitBox <gi...@apache.org>]

azagrebin commented on a change in pull request #9693: [FLINK-13984] Separate on-heap and off-heap managed memory pools
URL: https://github.com/apache/flink/pull/9693#discussion_r326673300
 
 

 ##########
 File path: flink-runtime/src/main/java/org/apache/flink/runtime/memory/MemoryManager.java
 ##########
 @@ -257,133 +216,98 @@ public boolean verifyEmpty() {
 	 *                                   of memory pages any more.
 	 */
 	public List<MemorySegment> allocatePages(Object owner, int numPages) throws MemoryAllocationException {
-		final ArrayList<MemorySegment> segs = new ArrayList<MemorySegment>(numPages);
-		allocatePages(owner, segs, numPages);
-		return segs;
+		List<MemorySegment> segments = new ArrayList<>(numPages);
+		allocatePages(owner, segments, numPages);
+		return segments;
 	}
 
 	/**
-	 * Allocates a set of memory segments from this memory manager. If the memory manager pre-allocated the
-	 * segments, they will be taken from the pool of memory segments. Otherwise, they will be allocated
-	 * as part of this call.
+	 * Allocates a set of memory segments from this memory manager.
+	 *
+	 * <p>The returned segments can have any memory type. The total allocated memory for each type will not exceed its
+	 * size limit, announced in the constructor.
 	 *
 	 * @param owner The owner to associate with the memory segment, for the fallback release.
 	 * @param target The list into which to put the allocated memory pages.
 	 * @param numPages The number of pages to allocate.
 	 * @throws MemoryAllocationException Thrown, if this memory manager does not have the requested amount
 	 *                                   of memory pages any more.
 	 */
-	public void allocatePages(Object owner, List<MemorySegment> target, int numPages)
-			throws MemoryAllocationException {
+	public void allocatePages(
+			Object owner,
+			Collection<MemorySegment> target,
+			int numPages) throws MemoryAllocationException {
 		// sanity check
-		if (owner == null) {
-			throw new IllegalArgumentException("The memory owner must not be null.");
-		}
+		Preconditions.checkNotNull(owner, "The memory owner must not be null.");
+		Preconditions.checkState(!isShutDown, "Memory manager has been shut down.");
 
 		// reserve array space, if applicable
 		if (target instanceof ArrayList) {
 			((ArrayList<MemorySegment>) target).ensureCapacity(numPages);
 		}
 
-		// -------------------- BEGIN CRITICAL SECTION -------------------
-		synchronized (lock) {
-			if (isShutDown) {
-				throw new IllegalStateException("Memory manager has been shut down.");
-			}
-
-			// in the case of pre-allocated memory, the 'numNonAllocatedPages' is zero, in the
-			// lazy case, the 'freeSegments.size()' is zero.
-			if (numPages > (memoryPool.getNumberOfAvailableMemorySegments() + numNonAllocatedPages)) {
-				throw new MemoryAllocationException("Could not allocate " + numPages + " pages. Only " +
-						(memoryPool.getNumberOfAvailableMemorySegments() + numNonAllocatedPages)
-						+ " pages are remaining.");
-			}
-
-			Set<MemorySegment> segmentsForOwner = allocatedSegments.get(owner);
-			if (segmentsForOwner == null) {
-				segmentsForOwner = new HashSet<MemorySegment>(numPages);
-				allocatedSegments.put(owner, segmentsForOwner);
-			}
+		// in the case of pre-allocated memory, the 'numNonAllocatedPages' is zero, in the
 
 Review comment:
   Good catch, thanks, I will change it.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services