-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8368825: Use switch expression for DateTimeFormatterBuilder pattern character lookup #26634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back swen! A progress list of the required criteria for merging this PR into |
|
@wenshao This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 835 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
You can use a table of size 64, and access the table with |
This will save memory space, but it will also increase the complexity a little bit. @Stable
private static final TemporalField[] FIELD_MAP = new TemporalField[64];
static {
// SDF = SimpleDateFormat
FIELD_MAP['G' & 0x3f] = ChronoField.ERA; // SDF, LDML (different to both for 1/2 chars)
FIELD_MAP['y' & 0x3f] = ChronoField.YEAR_OF_ERA; // SDF, LDML
FIELD_MAP['u' & 0x3f] = ChronoField.YEAR; // LDML (different in SDF)
FIELD_MAP['Q' & 0x3f] = IsoFields.QUARTER_OF_YEAR; // LDML (removed quarter from 310)
FIELD_MAP['q' & 0x3f] = IsoFields.QUARTER_OF_YEAR; // LDML (stand-alone)
FIELD_MAP['M' & 0x3f] = ChronoField.MONTH_OF_YEAR; // SDF, LDML
FIELD_MAP['L' & 0x3f] = ChronoField.MONTH_OF_YEAR; // SDF, LDML (stand-alone)
FIELD_MAP['D' & 0x3f] = ChronoField.DAY_OF_YEAR; // SDF, LDML
FIELD_MAP['d' & 0x3f] = ChronoField.DAY_OF_MONTH; // SDF, LDML
FIELD_MAP['F' & 0x3f] = ChronoField.ALIGNED_WEEK_OF_MONTH; // SDF, LDML
FIELD_MAP['E' & 0x3f] = ChronoField.DAY_OF_WEEK; // SDF, LDML (different to both for 1/2 chars)
FIELD_MAP['c' & 0x3f] = ChronoField.DAY_OF_WEEK; // LDML (stand-alone)
FIELD_MAP['e' & 0x3f] = ChronoField.DAY_OF_WEEK; // LDML (needs localized week number)
FIELD_MAP['a' & 0x3f] = ChronoField.AMPM_OF_DAY; // SDF, LDML
FIELD_MAP['H' & 0x3f] = ChronoField.HOUR_OF_DAY; // SDF, LDML
FIELD_MAP['k' & 0x3f] = ChronoField.CLOCK_HOUR_OF_DAY; // SDF, LDML
FIELD_MAP['K' & 0x3f] = ChronoField.HOUR_OF_AMPM; // SDF, LDML
FIELD_MAP['h' & 0x3f] = ChronoField.CLOCK_HOUR_OF_AMPM; // SDF, LDML
FIELD_MAP['m' & 0x3f] = ChronoField.MINUTE_OF_HOUR; // SDF, LDML
FIELD_MAP['s' & 0x3f] = ChronoField.SECOND_OF_MINUTE; // SDF, LDML
FIELD_MAP['S' & 0x3f] = ChronoField.NANO_OF_SECOND; // LDML (SDF uses milli-of-second number)
FIELD_MAP['A' & 0x3f] = ChronoField.MILLI_OF_DAY; // LDML
FIELD_MAP['n' & 0x3f] = ChronoField.NANO_OF_SECOND; // 310 (proposed for LDML)
FIELD_MAP['N' & 0x3f] = ChronoField.NANO_OF_DAY; // 310 (proposed for LDML)
FIELD_MAP['g' & 0x3f] = JulianFields.MODIFIED_JULIAN_DAY;
private void parsePattern(String pattern) {
// ...
TemporalField field = FIELD_MAP[cur & 0x3f];
// ...
} |
Webrevs
|
|
A sensible minor optimization. Looking at this code, I think to accomplish real performance gains, we might need some redesign to simplify the branchiness and speed up lookup. |
|
The suggestion, from Claes, to use and expression switch would work well too. |
|
j.t.f.DateTimeFormatterBuilder#parsePattern This is a large method with codeSize 1300+. I think optimizing it should be a separate PR. |
|
We might just remove this map when we optimize that huge method, which is this map's only user. |
RogerRiggs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
Show resolved
Hide resolved
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
Show resolved
Hide resolved
|
/integrate |
|
Going to push as commit 2c7f738.
Your commit was automatically rebased without conflicts. |
The DateTimeFormatterBuilder::FIELD_MAP previously used a Map<Character, TemporalField> for mapping pattern characters to TemporalField
instances. This PR refactors that implementation to use a switch expression instead, which eliminates the need to hold a Map in
memory.
The switch expression approach offers these advantages:
This change maintains the same functionality while improving the memory efficiency of pattern character lookup in
DateTimeFormatterBuilder by eliminating the static Map that was previously used for character-to-field mapping.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26634/head:pull/26634$ git checkout pull/26634Update a local copy of the PR:
$ git checkout pull/26634$ git pull https://git.openjdk.org/jdk.git pull/26634/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26634View PR using the GUI difftool:
$ git pr show -t 26634Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26634.diff
Using Webrev
Link to Webrev Comment