Open winfriedgerlach opened 6 days ago
Correct, all use of StringBuffer
is accidental/historical left-over: nothing in core parser/generator is thread-safe (minus some of SymbolTable
reuse which is explicitly synchronized as necessary) -- they are not meant to be used from multiple threads.
Cool stuff, will go through PRs.
More details regarding System.arrayCopy()
vs. Arrays.copyOf()
or .clone()
:
Very old comments (Joshua Bloch) say .clone()
is the fastest way to clone an array. This is already challenged in some of the answers to that question. Most published benchmarks of "medium age" (e.g. some years ago) find that System.arrayCopy()
is fastest.
Things seem to have changed quite a lot in the JDK recently (~beginning of 2023), so different Java versions may perform considerably differently:
Arrays.copyOf()
performance, see e.g. https://bugs.openjdk.org/browse/JDK-8301958System.arrayCopy()
slightly ahead of .clone()
, cf. https://bugs.openjdk.org/browse/JDK-8302315 and https://gist.github.com/schlosna/975e26965ec822ad42034b3ea2b08676Other quite new (2024) benchmarks find no significant difference between Arrays.copy()
and System.arrayCopy()
(note the absence of the JVM version under test...):
https://www.baeldung.com/java-system-arraycopy-arrays-copyof-performance
My own micro (and a little less micro) benchmarks showed System.arrayCopy()
ahead most of the times, sometimes on same level as clone()
/Arrays.copyOf()
.
As Jackson currently supports every Java version from Java 8 to 23, I recon it is safer to stick with System.arrayCopy()
for now when optimizing for performance, even if there is probably almost no performance difference when using the most current (=2024) JVM versions.
I definitely have to agree that the code is nicer with Arrays.copyOf()
though, see https://github.com/FasterXML/woodstox/commit/cfe59fbe0b4dbadf1941406759cd6875ad689b56 .
@cowtowncoder in StringVector.addString()
/.addStrings()
I stumbled over the array size being increased by 200%:
oldSize + (oldSize << 1)
This looked a bit odd to me, as in many other places in the source code you increase array size by 50% only, e.g. DataUtil.growArrayBy50Pct()
:
len + (len >> 1)
Is this intentional or maybe a typo (<<
instead of >>
) in StringVector
?
Whoa! That sounds more like a bug indeed: unless some comment explicitly states otherwise, and I don't there is.
So it should be shift-right to get +50% increase.
Woodstox already features impressive performance optimizations, to which I would like to add small ones. I will submit in individual PRs, so we can discuss the changes separately.
I benchmarked with a JMH test using namespace-aware StAX-parsing with very little value extraction, other use-cases may show less significant improvements (but still benefit slightly).
WstxInputData.isNameStartChar(char)
andWstxInputData.isNameStartChar(char)
as hotspots. I suggest to make the common case (ASCII) slightly faster there by using aboolean
lookup table. This takes ~256 bytes of extra memory, but I suppose that this a reasonable trade-off. I benchmarked with different JVM versions and results varied, but my JMH test showed improvements of e.g. ~3%, sometimes better.StringBuffer
withStringbuilder
. The latter is well-known to be faster due to lack of synchronization. If my analysis is correct,Stringbuffer
is never used in a place where thread-safety is relevant, so it can safely be replaced. This has already been done instax2-api
, just a leftover in woodstox I guess.Bucket
should be fine and faster than calling getters.Approaches that did not work (maybe someone else tries with different results?):
Arrays.copyOf()
or just.clone()
(called byArrays.copyOf()
in newer Java versions) instead ofSystem.arrayCopy()
for cloning arrays: While theoretically.clone()
should be fastest (also according to Joshua Bloch in a quite old statement), my microbenchmarking showed no significant difference, oftenSystem.arrayCopy()
was even faster. But the code reads nicer (shorter, easier to understand), I have to admit. (more details see comment below)(1 << currToken) & MASK_GET_TEXT_XXX
) also for masks with just 2 different types. This seems to perform worse than justtype == A || type == B
.Attribute.getQName()
seems to have optimization potential, but it's not a hotspot.