Skip to content
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

Performance tweaks #220

Open
winfriedgerlach opened this issue Nov 27, 2024 · 4 comments
Open

Performance tweaks #220

winfriedgerlach opened this issue Nov 27, 2024 · 4 comments

Comments

@winfriedgerlach
Copy link
Contributor

winfriedgerlach commented Nov 27, 2024

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).

  • Profilers identify WstxInputData.isNameStartChar(char) and WstxInputData.isNameStartChar(char) as hotspots. I suggest to make the common case (ASCII) slightly faster there by using a boolean 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.
  • A low hanging fruit is replacing StringBuffer with Stringbuilder. 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 in stax2-api, just a leftover in woodstox I guess.
  • Direct field access in (now private) inner class Bucket should be fine and faster than calling getters.

Approaches that did not work (maybe someone else tries with different results?):

  • Use Arrays.copyOf() or just .clone() (called by Arrays.copyOf() in newer Java versions) instead of System.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, often System.arrayCopy() was even faster. But the code reads nicer (shorter, easier to understand), I have to admit. (more details see comment below)
  • Use bit mask trick ((1 << currToken) & MASK_GET_TEXT_XXX) also for masks with just 2 different types. This seems to perform worse than just type == A || type == B.
  • Attribute.getQName() seems to have optimization potential, but it's not a hotspot.
winfriedgerlach added a commit to winfriedgerlach/woodstox that referenced this issue Nov 27, 2024
winfriedgerlach added a commit to winfriedgerlach/woodstox that referenced this issue Nov 27, 2024
winfriedgerlach added a commit to winfriedgerlach/woodstox that referenced this issue Nov 27, 2024
winfriedgerlach added a commit to winfriedgerlach/woodstox that referenced this issue Nov 27, 2024
cowtowncoder added a commit to winfriedgerlach/woodstox that referenced this issue Nov 27, 2024
cowtowncoder added a commit to winfriedgerlach/woodstox that referenced this issue Nov 27, 2024
cowtowncoder added a commit to winfriedgerlach/woodstox that referenced this issue Nov 27, 2024
cowtowncoder added a commit to winfriedgerlach/woodstox that referenced this issue Nov 27, 2024
@cowtowncoder
Copy link
Member

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.

winfriedgerlach added a commit to winfriedgerlach/woodstox that referenced this issue Nov 28, 2024
@winfriedgerlach
Copy link
Contributor Author

winfriedgerlach commented Nov 29, 2024

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:

Other 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 cfe59fb .

@winfriedgerlach
Copy link
Contributor Author

winfriedgerlach commented Nov 29, 2024

@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?

@cowtowncoder
Copy link
Member

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.

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

No branches or pull requests

2 participants