pjfanning opened a new pull request, #1317:
URL: https://github.com/apache/poi/pull/1317
Follow-up to the evaluator performance review: item 1, plain-cell references.
## What changed
A non-formula cell referenced by several formulas was read from the workbook
once per referencing formula — `getCell` (HashMap + `CellKey` in XSSF),
`getCellType`, `Double.parseDouble` of the raw `<v>` text, a fresh
`NumberEval`/`StringEval` — and only *then* was the plain value cache
consulted, solely to run the `areValuesEqual` sanity check against what had
just been read (the `TODO` in `EvaluationCache` already proposed dropping it).
`WorkbookEvaluator.evaluateReference` now asks the tracker for the cached
plain value first (`EvaluationTracker.getCachedPlainValue` →
`EvaluationCache.getPlainValueEntry(book, sheet, row, col)`) and returns it,
registering the dependency on the consuming frame exactly as before. Only a
miss reads the cell and creates the entry (`getOrCreatePlainValueEntry`). Blank
cells were never cached and still aren't; the `IStabilityClassifier` "final"
path is unchanged (no caching, no lookup). `IEvaluationListener` events
(`onReadPlainValue` on first read, `onCacheHit` after) fire in the same
sequence as before, so `TestEvaluationCache`'s logs are unchanged.
The original `getPlainValueEntry(int, int, int, int, ValueEval)` is kept
with its body intact, `@Deprecated` + `@Removal(version = "7.0.0")`; new
methods carry `@since 6.0.0`.
## Bug fixed on the way
`EvaluationCache.notifyDeleteCell` cleared the formulas depending on a
deleted plain cell but left the cell's own entry in the plain cache. Before,
that showed up as `IllegalStateException("value changed")` if the location was
later given a new value and read again (this is the exception the row/column
tests hit when `clearAllCachedResultValues()` was skipped); with cache-first
reads it would have served the stale value — four of the
`BaseTestFormulaEvaluatorCacheNotification`/`RowsAndColumns` delete tests
caught it. The entry is now removed, so the deleted cell reads as blank.
## Behaviour change to be aware of
An **un-notified** change to a plain cell is now invisible to formulas that
are re-evaluated for other reasons (they get the cached value), instead of
sometimes throwing `value changed`. That matches how formula cells already
behave and how the `notify*` contract is documented;
`TestEvaluationCache.testPlainValueServedFromCache` pins it down (first read →
`value`, later readers → `hit`, un-notified change ignored, `notifyUpdateCell`
serves the new value, `notifyDeleteCell` evicts, a later value is read afresh).
## XSSF benchmark (`poi-ooxml-benchmark`, back-to-back runs on the same
machine, `-Pjmh.profilers=gc`)
| rows | stability | trunk ms/op | this PR ms/op | trunk B/op | this PR B/op
|
|---|---|---|---|---|---|
| 100 | DEFAULT | 13.82 ± 0.27 | **12.46 ± 0.42** | 9.87 MB | 9.76 MB |
| 100 | TOTALLY_IMMUTABLE | 12.48 ± 0.47 | 12.60 ± 0.27 | 9.17 MB | 9.19 MB |
| 200 | DEFAULT | 28.33 ± 1.02 | **25.72 ± 3.41** | 19.83 MB | 19.34 MB |
| 200 | TOTALLY_IMMUTABLE | 25.52 ± 2.15 | 25.96 ± 1.17 | 18.49 MB | 18.19
MB |
About −10% time for `DEFAULT`, which now costs the same as
`TOTALLY_IMMUTABLE`; `TOTALLY_IMMUTABLE` is unchanged, as expected (it never
caches). Allocation barely moves because the invoice workbook rarely re-reads a
plain cell — each row's formulas read that row's inputs, and the shared reads
are the small Prices table (`VLOOKUP`) and `Params`. Workloads where many
formulas read the same inputs (lookups over a large table, `SUMIF`/`COUNTIF`
over the same range from many rows) are where the saving grows: those reads
were O(formulas × range) workbook accesses and are now O(range).
The benchmark `build.gradle` files gained `-Pjmh.profilers=gc` so the
allocation column can be reproduced.
`changes.xml` left for you.
Locally green: `ss.formula.*`, `TestHSSFFormulaEvaluator*`,
`TestFormulaEvaluatorBugs`, `TestBugs`, `ss.usermodel.*` (poi);
`TestXSSFFormulaEvaluator*`, `TestFormulaEvaluatorOnXSSF`,
`TestMultiSheetFormulaEvaluatorOnXSSF`, `TestSXSSFFormulaEvaluation`,
`ss.formula.*` (poi-ooxml).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
