jfboeuf opened a new issue, #3563:
URL: https://github.com/apache/maven-surefire/issues/3563
### New feature, improvement proposal
With `runOrder=balanced` (or `failedfirst`),
`StatisticsReporter.testSetCompleted()` re-serialises the whole run-statistics
map to disk after every test class, under a lock shared by all forks, on the
path that hands the fork its next class. With many forks and many short
classes, every class completion queues behind every other fork's full rewrite:
forks sit idle waiting for their next class while Maven spends a core rewriting
the same file.
## Suggested fixes (any one helps; they combine)
1. **Make `serialize()` cheaper** (small, local, low-risk change): skip the
per-entry `Scanner` when the method name has no line break (the common case:
none of the 16,515 entries in our file has one; the `Scanner` path from
SUREFIRE-1546 is still needed for multi-line display names). On our real
statistics file this takes `serialize()` from ~77 ms to ~7 ms per call (~10×),
with byte-identical output. It lowers the cost of each rewrite but keeps one
rewrite per class completion under the shared lock.
Implemented, with tests, on branch
[`jfboeuf:statistics-serialize-fast-path`](https://github.com/jfboeuf/maven-surefire/tree/statistics-serialize-fast-path)
(commit
[`0d74966`](https://github.com/jfboeuf/maven-surefire/commit/0d7496623b6c24851f528390543be1bf23be1516)).
The fast path checks for every line separator `Scanner.nextLine()` recognises
(`\r\n`, `\n`, `\r`, `\u2028`, `\u2029`, `\u0085`), so the file content is
unchanged for any method name; a test compares the output with the previous
`Scanner`-based implementation byte for byte. The fields of each line are also
written straight to the writer instead of being concatenated into a temporary
`String`.
2. **Write the statistics file once, when the execution completes**, instead
of after every class. This removes the write from the dispatch path entirely.
⚠ **This is a behaviour change, proposed deliberately.** Today
`serialize()` writes only `newResults`, i.e. the classes run so far in the
current execution (`StatisticsReporter.java:39-41`, `:57-58`), so if Maven is
interrupted (Ctrl-C, killed) the file is left holding a partial run, and the
history of every class not yet run is lost; on the next run those classes are
unknown, get priority 0 and are dispatched first. With a single write at the
end, an interrupted build leaves the **previous** file untouched: the results
of the interrupted run are lost, but the history of the last complete run is
kept, which we think is the better outcome for `balanced`. The trade-offs we
see: with `failedfirst`, failures of an interrupted run are not recorded, so
the next run does not put them first; and a first run that is interrupted
produces no file at all. A throttled periodic write could be added to narrow
these gaps, at the cost of bringing back partial files.
3. **Move the write off the per-fork consumer thread** (e.g. a single-thread
executor that coalesces pending writes), so dispatch of the next class never
waits for it. This is also a behaviour change (the file lags behind the run,
and pending writes are lost if Maven is killed) and adds a thread to manage,
for little gain over (1)+(2).
None of these reopens SUREFIRE-1244 (forks reading the statistics file while
another fork rewrote it): (1) does not touch the locking, (2) leaves a single
write, and (3) keeps a single writer thread.
I can open a PR for (1) from the branch above as a standalone change, and
can provide one for (2) if the behaviour change above is acceptable.
--
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]
