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

Frequent invocation of javax.xml.stream.XMLOutputFactory::newInstance negatively impacts performance #474

Closed
robsman opened this issue Sep 8, 2016 · 7 comments

Comments

@robsman
Copy link
Contributor

robsman commented Sep 8, 2016

While profiling a client application, it showed that there are lots of javax.xml.stream.XMLOutputFactory.newInstance() invocations, specifically when a client retrieves extracted document data from search results. In our use case, this almost totaled to the same amount of CPU time as the actual search.

@robsman
Copy link
Contributor Author

robsman commented Sep 8, 2016

I have applied a hotfix in the client project but still need to beef it up into a proper pull request.

The problematic code in our use case is in Utilities::writeEvents. However, there are other places that retrieve XML factories, e.g. SearchHandle::receiveContent, StructuredQueryBuilder::makeSerializer and SearchResponseImpl::handleMetrics.

image

My suggestion is to consolidate retrieval and caching of expensive factories into a utility class (maybe XmlFactoriesAccess ?) and adjust client code accordingly.

@sammefford
Copy link
Contributor

Thanks Rob!

@sammefford
Copy link
Contributor

We'll let you submit your patch first, then we'll use that as a model for the rest.

@robsman
Copy link
Contributor Author

robsman commented Oct 24, 2016

Hi Sam, after some further research it turned out the various XML factories are not guaranteed to be thread-safe. To address this I have reworked pull request #477 to use instance-per-thread caching.

The code is a bit verbose as the develop-3.0 branch is still Java 6. But nevertheless, it should be relatively straightforward.
We're running a copy of this in our project for some days now, so far, so good.

@sammefford
Copy link
Contributor

Thanks Rob!

@sammefford
Copy link
Contributor

@georgeajit as long as this didn't create any issues in unit tests, I think we're good on this

@georgeajit georgeajit added ship and removed test labels Jan 25, 2018
@georgeajit
Copy link
Contributor

Don't see any regressions as of now (01/25/2018) on the develop branch. Shipping this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants