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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions src/main/java/org/codehaus/staxmate/dom/DOMConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,30 @@ public Document buildDocument(XMLStreamReader r)
throws XMLStreamException
{
// Let's enable namespace awareness by default
return buildDocument(r, true);
}

/**
* This method will create a {@link org.w3c.dom.Document} instance using
* the default JAXP DOM document construction mechanism and
* populated using the given StAX stream reader.
*<p>
* Note: underlying stream reader will not be closed by calling this
* method.
*
* @param r Stream reader from which input is read.
* @param namespaceAware Is namespace-awareness enabled.
* @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 •
edited
Loading

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.

public Document buildDocument(XMLStreamReader r, boolean namespaceAware)
throws XMLStreamException
{
DocumentBuilder db;
try{
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
dbf.setNamespaceAware(true);
dbf.setNamespaceAware(namespaceAware);
db = dbf.newDocumentBuilder();
} catch (ParserConfigurationException pce) {
throw new XMLStreamException(pce);
Expand Down Expand Up @@ -571,4 +591,3 @@ public String getQualified(String prefix, String localName)
}
}
}