Skip to content

feat: cache date and number formatters#948

Open
Duansg wants to merge 2 commits into
apache:mainfrom
Duansg:improve-perf
Open

feat: cache date and number formatters#948
Duansg wants to merge 2 commits into
apache:mainfrom
Duansg:improve-perf

Conversation

@Duansg

@Duansg Duansg commented Jul 14, 2026

Copy link
Copy Markdown
Member

Purpose of the pull request

DateUtils and NumberUtils rebuilt a formatter on every cell. This PR caches them per thread, keyed by locale + pattern, so a formatter is built once and reused.

The java.util.Date path in the very same class was already cached; the java.time and DecimalFormat paths just never got the same treatment. This PR closes that gap.

What's changed?

Both caches are ThreadLocal (not a shared static map) so they are bounded by thread lifetime and released by the existing cleanup, consistent with the sibling DATE_FORMAT_THREAD_LOCAL.

  1. DateUtils — new DATE_TIME_FORMATTER_THREAD_LOCAL cache; all 8 sites resolve through getCacheDateTimeFormat(pattern, locale). Cleared in the existing removeThreadLocalCache().
  2. NumberUtils — new DECIMAL_FORMAT_THREAD_LOCAL cache via getCacheDecimalFormat(pattern, roundingMode), plus a new removeThreadLocalCache() wired into the read/write finish paths (WriteContextImpl, ExcelAnalyserImpl).

Benchmark

Per cell (JMH 1.37, -prof gc, 2 forks; allocation deterministic ±0.001 B/op)

┌──────────────┬─────────────┬────────────┬─────────────┬──────────────┬─────────────┬─────────┐
│  Benchmark   │ Time before │ Time after │   Time Δ    │ Alloc before │ Alloc after │ Alloc Δ │
├──────────────┼─────────────┼────────────┼─────────────┼──────────────┼─────────────┼─────────┤
│ dateFormat   │    422.8 ns │   205.4 ns │        −51% │        776 B │       396 B │    −49% │
├──────────────┼─────────────┼────────────┼─────────────┼──────────────┼─────────────┼─────────┤
│ dateParse    │    680.0 ns │   631.1 ns │        −7%  │       1272 B │       880 B │    −31% │
├──────────────┼─────────────┼────────────┼─────────────┼──────────────┼─────────────┼─────────┤
│ numberFormat │    905.2 ns │   749.4 ns │        −17% │       1656 B │       480 B │    −71% │
├──────────────┼─────────────┼────────────┼─────────────┼──────────────┼─────────────┼─────────┤
│ numberParse  │    492.8 ns │   277.0 ns │        −44% │       1568 B │       376 B │    −76% │
└──────────────┴─────────────┴────────────┴─────────────┴──────────────┴─────────────┴─────────┘

End-to-end (500000-row CSV, median of 3)

┌───────┬───────────────────┬──────────┬──────────┬──────┐
│ Phase │      Metric       │  Before  │  After   │  Δ   │
├───────┼───────────────────┼──────────┼──────────┼──────┤
│ Write │ Thread allocation │ 2,386 MB │ 1,593 MB │ −33% │
├───────┼───────────────────┼──────────┼──────────┼──────┤
│ Read  │ Thread allocation │ 1,960 MB │ 1,279 MB │ −35% │
└───────┴───────────────────┴──────────┴──────────┴──────┘

Checklist

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

@delei delei requested a review from Copilot July 15, 2026 00:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves formatting performance by caching date/time and numeric formatter instances in thread-local maps keyed by locale + pattern, avoiding per-cell formatter construction and ensuring caches are cleared at the end of read/write lifecycles.

Changes:

  • Added a ThreadLocal<Map<String, DateTimeFormatter>> cache in DateUtils and routed java.time parse/format through a cached getCacheDateTimeFormat(...).
  • Added a ThreadLocal<Map<String, DecimalFormat>> cache in NumberUtils, with cleanup wired into both write and read finish paths.
  • Expanded unit tests to validate cache behavior across default FORMAT locale changes and to verify thread-local cleanup.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/DateUtils.java Adds a per-thread DateTimeFormatter cache and clears it in removeThreadLocalCache().
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/NumberUtils.java Adds a per-thread DecimalFormat cache keyed by default FORMAT locale + pattern, plus public cache cleanup.
fesod-sheet/src/main/java/org/apache/fesod/sheet/context/WriteContextImpl.java Calls NumberUtils.removeThreadLocalCache() during write finish cleanup.
fesod-sheet/src/main/java/org/apache/fesod/sheet/analysis/ExcelAnalyserImpl.java Calls NumberUtils.removeThreadLocalCache() during read finish cleanup.
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/DateUtilsTest.java Adds tests covering locale-sensitive DateTimeFormatter caching and cleanup verification.
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/NumberUtilsTest.java Adds tests covering locale-sensitive DecimalFormat caching and cleanup across read/write finishes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +95 to +96
@Test
void test_formatCache_tracksDefaultFormatLocaleChanges() {
Comment on lines +197 to +198
@Test
void test_dateTimeFormatterCache_distinguishesRootFromDefaultLocale() {
Comment on lines +216 to +217
@Test
void test_dateTimeFormatterCache_tracksDefaultFormatLocaleChanges() {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants