Refactor and simplify Inliner's weighCallSite routine

[ymanton]

This PR contains a series of patches that try to make TR_MultipleCallTargetInliner::weighCallSite a little easier to work with in the future. Probably easier to review commit by commit. Note that the heavy lifting for 2582e0b was done by AI.

Note that a handful of methods that were previously not subjected to the full processing of weighCallSite() will now go through the path that all other methods take. The methods are:

java_lang_Class_newInstance
java_util_Arrays_fill
java_util_Arrays_equals
java_lang_String_equals
sun_io_ByteToCharSingleByte_convert
sun_io_ByteToCharDBCS_EBCDIC_convert
sun_io_CharToByteSingleByte_convert
sun_io_ByteToCharSingleByte_JITintrinsicConvert
java_math_BigDecimal_subMulAddAddMulSetScale

The bytecode sizes of these methods were estimated (and along the way other facts learned), but they were then given a fixed size of 10 and the size was not scaled up/down based on other heuristics. The refactoring will cause them to still be given a size of 10 but the size will then go through the rest of the heuristics, and they could potentially be removed from the list of inlining candidates if they exceed the size threshold.

[ymanton]

Added 363e06c which is a fix for something I believe to be a bug. When scaling the estimated size of a candidate for inlining based on frequency and the number of estimated calls nested in the candidate, the calculation inadvertently ignores the number of estimated calls and uses a value derived from the frequency twice. This doesn't appear to be the intention of the code at all.

[mpirvu]

I am suggesting @nbhuiyan and @vijaysun-omr as reviewers.
We will need to run some throughput oriented benchmarks to make sure there are no regressions.

[nbhuiyan]

This is a very important piece of work in my opinion, and a move in the right direction. I have a few comments in this first round (of many!) review of your changes.

[nbhuiyan]

This does not seem to be mentioned in the commit msg, and also not defined/used anywhere.

[ymanton]

This was from a later patch that isn't part of this PR. Removed, thanks.

[nbhuiyan]

This might fail the functionality-preserving claim of the refactoring work. The original state was that these recognized methods would go through ECS and then upon return, were being set as calltarget->_partialInline->setCallNodeTreeTop(callNodeTreeTop);, followed by a fixed size 10, and then skip all of the stuff in the else block of the original code. However, they now get various adjustments in the original code's else block that were previously skipped (for example, cold block frequency scaling). One easy way around this might be to add all these methods to alwaysWorthInlining so that the scaled frequency does not impact their inlining outcomes.

Edit: I read the PR description discussing this earlier, but forgot about it when I was doing the review, where you pointed that out. However, my comment is still the same regarding the fact that this refactoring has a functional change that's not documented in the commit message.

[nbhuiyan]

If the intention behind having special handling of these few methods was to always inline them, then maybe they could be removed entirely from weighCallSite and instead be all part of alwaysWorthInlining.

[ymanton]

Yeah, adding them to alwaysWorthInlining would make more sense since that is ultimately the intent here, but that's called in a few places where as this set of methods is only considered here. I'll test that out and see what it affects.

I've modified the commit message to remove the erroneous text.

[ymanton]

I've also moved

        if (calltarget->_isPartialInliningCandidate && calltarget->_partialInline)
            calltarget->_partialInline->setCallNodeTreeTop(callNodeTreeTop);

out of and after estimateCallTargetSize() since it has nothing to do with size estimation and is done for both the recognized methods and all other methods when ecs->calculateCodeSize(...) == true.

[nbhuiyan]

Yeah, adding them to alwaysWorthInlining would make more sense since that is ultimately the intent here, but that's called in a few places where as this set of methods is only considered here. I'll test that out and see what it affects.

Any update on this?

[nbhuiyan]

Interestingly, the original error actually shielded us from a potential issue. ecs->getNumOfEstimatedCalls() gets us _numOfEstimatedCalls, which starts at 0. The only way it increases is if method being estimated has callees to analyse recursively, which is the only thing that increments that field. So for a leaf method that was analyzed by ECS, the value returned would be 0, so we have a division by zero here, so now numCallsFactor will be +infinity in many instances. Then the size is obtained multiplying infinity with factor and size, followed by an int cast, which I believe is undefined if numCallsFactor was infinity.

The fix would be first checking if numEstimatedCalls is >= 1.

[ymanton]

Good catch, thanks. I've changed the denominator to be 1 + ecs->getNumOfEstimatedCalls() since if the call target has no nested calls size should be estimated entirely from the call target itself and avgMethodSize should be equal to size.

[nbhuiyan]

Thinking about this again, in a scenario where we have a method with size 2000 at this point, with numEstimatedCalls() 0, then the numCallsFactor would be 18. Let's say the frequency is 9000+, so this would be a candidate we would want to inline, and therefore scaled down. In this case, what happens is the opposite of what this code is claiming to do. We multiply 2000 * factor * 18, and let's say the factor is 0.4f, so then it results in size getting blown up by 7x in a place that intends to reduce size of such callees. The tables you shared demonstrate that pattern and the undefined cuttoff point, and I think it highlights the need for some sort of adjustments we should make. The purpose of this path is to reduce sizes, it should not also be silently blowing up sizes in some scenario like the one I just described, and all that method would need to not get blown up in size is to have 100+ nested callees, which does not make sense to me.

The reason we need to be extra careful with this is that this change is going to have a big impact on inlining outcomes across the board, as the buggy equation that always scaled down large callees is now changed to exclude lots of callees that were not blown up in size before.

Let me know your thoughts on this.

[ymanton]

Regarding the size scaling based on getNumOfEstimatedCalls(), in the interest of understanding the previous behavior and the new behaviour, I created some tables that show the scaled size based on input sizes and number of calls for three different frequencies.

Green numbers indicate size was scaled down, inlining made more likely.
Red numbers indicate size was scaled up, inlining made less likely.

For frequency=1000

Original buggy equation:

Fixed equation:

For frequency=5000:

For frequency=9000:

The old buggy equation scaled the size down under regardless of the number of calls as expected, and quite aggressively based on frequency alone.

The fixed equation scales size down as frequency and number of calls increase, up to a cutoff point, and then scales size up. The behaviour looks sensible to me.

[vijaysun-omr]

jenkins test sanity.functional all jdk25

[vijaysun-omr]

I doubt the failures here are "real" since they were also seen in #24199 where I launched testing around the same time, but you can check and post.

[ymanton]

AIX build failure is an infra issue. Mac x86-64 test failure is an instance of #24214.

[nbhuiyan]

I have done another round of review, and have a few additional comments inline.

A nit: based on CONTRIBUTING.md:

In addition, individual commits that include GenAI authored content must attribute that at the end of the commit message (before any human authorship signatures).

Assisted-by: IBM Bob appears at the end of your commit fed73ea.

[nbhuiyan]

Yeah, adding them to alwaysWorthInlining would make more sense since that is ultimately the intent here, but that's called in a few places where as this set of methods is only considered here. I'll test that out and see what it affects.

Any update on this?

[nbhuiyan]

This is a pre-existing bug where we have a %p without the corresponding arg. Since this code is touched here it makes sense to fix that.

[ymanton]

Fixed via another patch on top.

[nbhuiyan]

Thinking about this again, in a scenario where we have a method with size 2000 at this point, with numEstimatedCalls() 0, then the numCallsFactor would be 18. Let's say the frequency is 9000+, so this would be a candidate we would want to inline, and therefore scaled down. In this case, what happens is the opposite of what this code is claiming to do. We multiply 2000 * factor * 18, and let's say the factor is 0.4f, so then it results in size getting blown up by 7x in a place that intends to reduce size of such callees. The tables you shared demonstrate that pattern and the undefined cuttoff point, and I think it highlights the need for some sort of adjustments we should make. The purpose of this path is to reduce sizes, it should not also be silently blowing up sizes in some scenario like the one I just described, and all that method would need to not get blown up in size is to have 100+ nested callees, which does not make sense to me.

The reason we need to be extra careful with this is that this change is going to have a big impact on inlining outcomes across the board, as the buggy equation that always scaled down large callees is now changed to exclude lots of callees that were not blown up in size before.

Let me know your thoughts on this.

[nbhuiyan]

nit: this should probably be declared with static, at least for consistency with other places in the JIT.

[ymanton]

Fixed, thanks.

[nbhuiyan]

This is answering whether something is a reflection method, not whether to skip. We should probably rename that.

[ymanton]

Renamed and tidied up the code inside and around its call site.

[nbhuiyan]

The comment could be fixed here to to describe that weighCallSite is going to terminate if <reason>

[ymanton]

I removed the comment altogether, it was added by AI for whatever reason and the code reads pretty clearly now after some tweaking, so such a comment would be low information at best.

It would be worth having a comment explaining at a higher level why bailing out of weighing the entire call site is appropriate if at least one target is a generated reflection method while the top-level method is not, but I couldn't figure out why after a bit of searching.

[ymanton]

A nit: based on CONTRIBUTING.md:

In addition, individual commits that include GenAI authored content must attribute that at the end of the commit message (before any human authorship signatures).

Assisted-by: IBM Bob appears at the end of your commit fed73ea.

Fixed, thanks.

Yeah, adding them to alwaysWorthInlining would make more sense since that is ultimately the intent here, but that's called in a few places where as this set of methods is only considered here. I'll test that out and see what it affects.

Any update on this?

(Can't respond to this inline, reproducing here.)

Most of these methods are obsolete and no longer exist in any JCL we support. The ones that are still relevant are:

java_lang_Class_newInstance
java_util_Arrays_fill
java_util_Arrays_equals
java_lang_String_equals

None of the above are relevant in the benchmarks I've been running, so I didn't observe any difference. It's possible that the above are important to some application or benchmark somewhere, but given that they appear together in a list with methods that haven't existed since before JDK8 and the inliner has (I assume) changed significantly since then I'd be inclined to just remove this code path altogether and treat these methods like any other until there's a fresh reason to do otherwise.

[ymanton]

Thinking about this again, in a scenario where we have a method with size 2000 at this point, with numEstimatedCalls() 0, then the numCallsFactor would be 18. Let's say the frequency is 9000+, so this would be a candidate we would want to inline, and therefore scaled down. In this case, what happens is the opposite of what this code is claiming to do. We multiply 2000 * factor * 18, and let's say the factor is 0.4f, so then it results in size getting blown up by 7x in a place that intends to reduce size of such callees. The tables you shared demonstrate that pattern and the undefined cuttoff point, and I think it highlights the need for some sort of adjustments we should make. The purpose of this path is to reduce sizes, it should not also be silently blowing up sizes in some scenario like the one I just described, and all that method would need to not get blown up in size is to have 100+ nested callees, which does not make sense to me.

The reason we need to be extra careful with this is that this change is going to have a big impact on inlining outcomes across the board, as the buggy equation that always scaled down large callees is now changed to exclude lots of callees that were not blown up in size before.

Let me know your thoughts on this.

Had a hard time understanding why you said "The purpose of this path is to reduce sizes, it should not also be silently blowing up sizes in some scenario like the one I just described" until I looked at the surrounding code. I assume your point is that the formula used inside the following check together with the accompanying comment

if (frequency > borderFrequency) {
        // High frequency: reduce size to encourage inlining

has no business scaling up the size under any circumstances, especially since the adjacent code handles the lower frequencies with alternative formulas.

I'd agree with that; sadly the bug was in some ways more true to the ultimate intention it seems. I didn't really pay attention to the other parts of that chain of if-else-ifs or whether the size scaling being done in each case makes sense, but I'll do that now and respond after.

[ymanton]

For reference, the scaling code handles frequencies in 4 buckets:

maxFrequency >= frequency > borderFrequency -> discussed in this PR; scale based on frequency and number of calls
borderFrequency >= frequency >= coldBorderFrequency -> no scaling
coldBorderFrequency > frequency >= veryColdBorderFrequency -> inverse linear w.r.t. frequency (size grows linearly as frequency decreases)
veryColdBorderFrequency > frequency >= 0 -> inverse quadratic w.r.t. frequency (size grows faster than linear as frequency decreases)

Based on that it certainly makes no sense for size to ever be scaled up if we're in the first bucket.

The easiest solution is to use a reciprocal-based formula that incorporates both frequency and number of calls:

normalizedFreq = frequency / maxFrequency;
numCalls = ecs->getNumOfEstimatedCalls();
scalingFactor = 1.0 / (1.0 + normalizedFreq * (1.0 + numCalls));
size = size * scalingFactor;

Scales based on frequency alone when numCalls==0, naturally clamped to the range [size,0).

At frequency=9000 it would look like this:

Alternatively, if we presume that number of calls as a factor has diminishing returns, we can use a slightly different formula that naturally reduces the influence of number of calls as that factor gets larger:

normalizedFreq = frequency / maxFrequency;
numCalls = ecs->getNumOfEstimatedCalls();
callFactor = log(1.0 + numCalls);
scalingFactor = 1.0 / (1.0 + normalizedFreq * (1.0 + callFactor));
size = size * scalingFactor;

Again when numCalls==0 it's the same as above (freq only scaling). As numCalls increases, callFactor increases slower, so 0->5 calls has a bigger impact than 20->50 calls.

At frequency=9000:

I'll test these alternatives and see what happens.