Reduce size of j.t.f.DateTimePrintContext::adjust

[jaikiran.pai at oracle.com]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20250805/8745e25e/attachment-0001.htm>