Fix size calculation logic in PooledByteBufAllocator

[laosijikaichele]

Motivation:

1. When set "io.netty.allocator.pageSize" to 4MiB or larger, the PooledByteBufAllocator.DEFAULT instance throws java.lang.IllegalArgumentException in the construction stage:

public class TestDemo {
    static {
        System.setProperty("io.netty.allocator.pageSize", (1 << 22) + ""); // Set page size to 4MiB
    }

    public static void main(String[] args) {
        System.out.println(PooledByteBufAllocator.DEFAULT.metric()); // Throws `pageSize` overflow exception.
    }
}

• The pageSize for PooledByteBufAllocator.DEFAULT is initialized and validated in the static block, and it is supposed to resolve the overflow problem in the static block initialization stage.

• After the static block initialization finished, the PooledByteBufAllocator.DEFAULT should not throw pageSize overflow exception in the construction stage.

2. Some size-validation logics in the PooledByteBufAllocator's constructor and static block can be optimized.
   For example:
   Some validation code in the constructor should move into method validateAndCalculatePageShifts().

Modification:

Correct the size validation logic.

Result:

Fix the problem above, and optimize size-validation related code.

[jchrys]

(I'm not one of committer.) IMHO,for the issue # 1. I think it is correct behavior. The reason you're getting an error when using a 4MiB page size and the default maxOrder of 9 is because it causes an int32 overflow. To fix this, you can manually set the io.netty.allocator.maxOrder to a value less than 9.

By the way, if this issue is from your use cases, may I ask you why you need such large page and chunk sizes? AFAIK, using such huge chunk size almost certainly can degrade performance and make it harder to manage memory efficiently, with the current implementations.

[laosijikaichele]

using such huge chunk size almost certainly can degrade performance and make it harder to manage memory efficiently.

I agree with you above. But I take this issue from a pure code-logic perspective.

for the issue # 1. I think it is correct behavior. The reason you're getting an error when using a 4MiB page size and the default maxOrder of 9 is because it causes an int32 overflow. To fix this, you can manually set the io.netty.allocator.maxOrder to a value less than 9.

It is a correct behavior ONLY when the pageSize is passed directly to the constructor method as a parameter.
But the instance PooledByteBufAllocator.DEFAULT is NOT that case.

The pageSize value for PooledByteBufAllocator.DEFAULT is initialized and validated inside the static block, and the try...catch in the static block is supposed to take care of the overflow problem.

So the PooledByteBufAllocator.DEFAULT should not rise the pageSize overflow Exception when in construction stage.

The following code piece is from PooledByteBufAllocator 's static initializer block:

try {
      validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, defaultMaxOrder);
 } catch (Throwable t) {
      maxOrderFallbackCause = t;
      defaultMaxOrder = 9;
 }

The method validateAndCalculateChunkSize(DEFAULT_PAGE_SIZE, defaultMaxOrder) already did the overflow check and rise an Exception, then the catch block catch the Exception, and fall back the defaultMaxOrder to 9, at this point, the defaultMaxOrder = 9 is supposed to resolve the overflow problem, but it actually not, so the following code goes overflowed:

// `defaultChunkSize` overflowed to a negative value.
final int defaultChunkSize = DEFAULT_PAGE_SIZE << DEFAULT_MAX_ORDER;

So, we should solve the overflow problem in the catch block.

The overflow Exception in the PR's code sample is triggered in the PooledByteBufAllocator 's constructor, but it does not change the fact that the default static initializer's overflow problem should be take care earlier, which means in the static block initializer.

[jchrys]

I'm unsure if the proposed approach is the most suitable one for this situation. It seems that setting a large value for pageSize also affects the default value of maxOrder. Perhaps we should explore alternative solutions that include a fallback to the default page size.

[laosijikaichele]

It seems that setting a large value for pageSize also affects the default value of maxOrder. Perhaps we should explore alternative solutions that include a fallback to the default page size.

Based on current PR's committed code, this can be simply done by setting a lower MAX_PAGE_SIZE limit.
We can change the following code in method PooledByteBufAllocator.validateAndCalculatePageShifts().

from:

if (pageSize > MAX_CHUNK_SIZE) {
        throw new IllegalArgumentException("pageSize: " + pageSize + " (expected: " + MAX_CHUNK_SIZE + ')');
}

to:

if (pageSize > MAX_PAGE_SIZE) {
        throw new IllegalArgumentException("pageSize: " + pageSize + " (expected: " + MAX_PAGE_SIZE + ')');
}

If we set MAX_PAGE_SIZE to smaller than 4 MiB, then the default value of maxOrder(9) will not be affected.

If pageSize exceed MAX_PAGE_SIZE, a IllegalArgumentException exception will be raised and be caught in PooledByteBufAllocator's static block code, then the pageSize will fallback to default page size.

[jchrys]

Cool, Thank you for taking the time to reply to my comment. I would like to kindly request that we wait for the core developers to weigh in. Their expertise and input could provide valuable insights into this issue and guide us in making the best decision.

[franz1981]

There's some way to add a test to validate what's trying to solve?

[franz1981]

There's some way to add a test to validate what's trying to solve?

[chrisvest]

Hi, sorry for letting this hang. I had some comments.

[chrisvest]

I don't think we can use Pow2 directly because it violates OSGi encapsulation. Add an indirection method to PlatformDependent and call it that way.

[chrisvest]

Same here.

[chrisvest]

If I read this right, it will find the largest chunk size that pass validation. That could end up using a lot of memory. I think we should have more reserved heuristics beyond, say, 16 MiB chunks or there abouts, such that we try to keep memory usage down beyond that point.

[laosijikaichele]

@chrisvest If we limit the default max CHUNK size within 16 MiB, then should we also limit the default max PAGE size within a similar reasonable size?

[chrisvest]

I wasn't thinking to place a limit beyond what we already do, but rather have the algorithm put fewer pages in the chunks as the page size gets bigger.

[laosijikaichele]

@chrisvest , to approach this, I think the logic in method PooledByteBufAllocator.validateAndCalculateChunkSize(int pageSize, int maxOrder) also needs to be modified?

[chrisvest]

Yes, so we'd basically change the heuristic to find a balance between the two variables.

[chrisvest]

Talking about heuristics and reading through the comments, I'm inclined to come down on the side of being more strict and less clever.

In my opinion, the assertion that PooledByteBufAllocator.DEFAULT should just work only holds for systems where the default properties have not been modified. Those who go about modifying the default page size or max-order, need to do so with enough understanding to break the allocator.

What we should do, is place sufficient validation early in initialization, and produce useful error messages for validation failures.

If we paper over bad configurations with heuristics, we'd be pretending to know what people intended with the settings. I think it's better to fail early and explicitly, as an exception from a static initializer would.

[chrisvest]

Suggested change

	private static final int MAX_ORDER_UPPER_BOUNDER = 14;
	private static final int MAX_ORDER_UPPER_BOUND = 14;

The ER ending looks like a typo.

[chrisvest]

Yes, so we'd basically change the heuristic to find a balance between the two variables.

[laosijikaichele]

I think it's better to fail early and explicitly, as an exception from a static initializer would.

Agree, then we can remove the try...catch blocks in the static initializer, and let the exception throws once it failed the validation.