>From my perspective, the proposed change looks fine. Extracting out
error creation is a pretty common trick to get a performance gain
through additional inlining. I'd also note that it seems like the
method was identified as performance sensitive during the original
testing, as identified by the comments "early return is an
optimization".
Looking at the code now, I think the better inlining change would be
to introduce a new private method into the logic at the
common/uncommon point, something like this:
private static TemporalAccessor adjust(final TemporalAccessor
temporal, DateTimeFormatter formatter) {
// normal case first (early return is an optimization)
...
// ensure minimal change (early return is an optimization)
...
adjust0(...)
}
private static TemporalAccessor adjust0(...) {
// make adjustment
...
}
I suspect such a change would be more beneficial than the original proposal.
Stephen
On Tue, 5 Aug 2025 at 16:34, <jaikiran....@oracle.com> wrote:
>
> Hello Shaojin,
>
> Looking at the proposed change, the proposal here appears to be to replace an
> inline "new DateTimeException(...)" call with a call to a new private method
> which returns a "new DateTimeException(...)", to please the hotspot
> compiler's inlining decision.
>
> I think this isn't a useful change. Enabling the -XX:+PrintInlining will
> naturally print the runtime compiler's inlining decisions and there could be
> some/many logs which complain that the method couldn't be inlined. Changing
> the java code (like here) to closely handhold it to match the (unspecified)
> expectations of a particular (current) implementation of the runtime compiler
> shouldn't be the way to code in java language. Such changes, like the one
> here, won't help with the maintainability or the readability of the code.
>
> -Jaikiran
>
> On 05/08/25 6:55 am, wenshao wrote:
>
> By adding the JVM startup parameters `-XX:+UnlockDiagnosticVMOptions
> -XX:+PrintInlining` and analyzing the printed log information, and found that
> the code size of the j.t.f.DateTimePrintContext::adjust method is 382, which
> is greater than 325, causing inlining failure.
>
> Below is the log message:
> ```
> @ 7 java.time.format.DateTimePrintContext::adjust (382 bytes) failed to
> inline: hot method too big
> ```
>
> We can extract the exception-generating code into two smaller methods,
> reducing the code size from 382 to 322, allowing C2 to inline the
> DateTimePrintContext::adjust method.
>
> The refactored code looks like this:
> ```java
> private static TemporalAccessor adjust(final TemporalAccessor temporal,
> DateTimeFormatter formatter) {
> // ...
> if (overrideZone.normalized() instanceof ZoneOffset &&
> temporal.isSupported(OFFSET_SECONDS) &&
> temporal.get(OFFSET_SECONDS) !=
> overrideZone.getRules().getOffset(Instant.EPOCH).getTotalSeconds()) {
> throw unableApplyOverrideZone(temporal, overrideZone);
> }
> // ....
> if (f.isDateBased() && temporal.isSupported(f)) {
> throw unableApplyOverrideChronology(temporal, overrideChrono);
> // ...
> }
> private static DateTimeException
> unableApplyOverrideChronology(TemporalAccessor temporal, Chronology
> overrideChrono) {
> return new DateTimeException("Unable to apply override chronology '"
> + overrideChrono +
> "' because the temporal object being formatted contains date
> fields but" +
> " does not represent a whole date: " + temporal);
> }
> private static DateTimeException unableApplyOverrideZone(TemporalAccessor
> temporal, ZoneId overrideZone) {
> return new DateTimeException("Unable to apply override zone '" +
> overrideZone +
> "' because the temporal object being formatted has a
> different offset but" +
> " does not represent an instant: " + temporal);
> }
> ```
>
> I submitted a draft Pull Request https://github.com/openjdk/jdk/pull/26633 ,
> Hope to get your feedback.
>
> -
> Shaojin Wen
