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

Easier way to use namespace awareness: false #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Easier way to use namespace awareness: false #11

wants to merge 1 commit into from

Conversation

tomimas
Copy link

@tomimas tomimas commented Oct 27, 2017

Turning old functionality (even though it was against the documentation) back 'on' is easier, with this change

* @return <code>Document</code> - DOM document object.
* @throws XMLStreamException If the reader threw such exception (to
* indicate a parsing or I/O problem)
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I was the one that pushed the patch on this method yesterday and would like to give my feedback.
Even though the buildDocument methods that instantiates the factory/docBuilder are very convenient they can be harmful when called from big loops.
I think it would be nice to update the javadoc to warn the user to prefer the buildDocument(reader, DocumentBuilder) methods when calling from big loops.

I found yesterday's issue while doing something like:

while (reader.hasNext()) {
    reader.next();
    if (something) {
        converter.buildDocument(reader);
    }
}

Where buildDocument was being invoked more than 1 million times. It was taking more than 2 minutes to complete and consuming a lot of heap.
A silly small change of instantiating the documentBuilder before the loop and passing it to the method reduced the execution time from 2 minutes to 2 seconds and reduced drastically the heap usage. Something like:

DocumentBuilder db = getDocumentBuilder();
while (reader.hasNext()) {
    reader.next();
    if (something) {
        converter.buildDocument(reader, builder);
    }
}

Off course, the fact that the previous code was instantiating the factory twice was making it worse, but still a single instantiation has a impact that I would never imagine.

BTW, thank you guys for your great library.

cheers!
Francisco

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed, JDK builder factories are incredibly, stupidly expensive to construct. This is due to SPI discovery process that traverses classpath and is very slow for many deployments.
Typically instances are constructed into static members; I don't recall why this is/was not done here.
Especially as there are only 2 alternatives here (namespace-aware, not)

Copy link
Author

@tomimas tomimas Oct 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the current implementation is, because DocumentBuilder and DocumentBuilderFactory are not known to be thread safe. If DOMConverter have been constructed with a shared DocumentBuilder and have been used in threads to do e.g. XML magick for xml file content files, in this way it is still has maintained thread safety. It's seems more a hack than intended functionality.

Most often, in my experience namespace awareness is not really required. Sometimes changing the documentation based on what really is done, is better option than changing underlying code, because this feature have been around long time.

That is the reason I proposed this code change.

@cowtowncoder
Copy link
Member

From functionality perspective makes sense. However: would it make more sense, from API perspective, to require construction of namespace-aware (or not) instances, instead of changing build method(s)?
This should make it bit easier to both document the cost of constructing DOMConverter (which would create document builder factory, unless one was passed), and allow passing pre-configured factory.

@chicobento
Copy link
Contributor

Yes, just now I realized that there is a constructor for passing the docBuilder, and an empty constructor that will instantiate one.
But the _docBuilder is never used, which is even more confusing.
One option would be to change the buildDocument(XMLStreamReader r) to just use the _docBuilder.

@cowtowncoder
Copy link
Member

@chicobento I think this class came via contribution. It does sound like _docBuilder should indeed be used, no point in keeping one around otherwise.

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

Successfully merging this pull request may close these issues.

3 participants