Hi Joe,
answers inline.
Thanks for reporting and providing patch for the issue! Looks like a
nice solution that may potentially reduce memory requirement for some
large templates. Could you also verify that the patch also fixes
JDK-8150699 [1] that was created the same day as yours?
Yes, which coincidence. The issue basically is the same. I've picked the bug
and marked it as duplicate
I assume the stylesheet is created to just illustrate the issue. If it's
a real use case, then it should have made the variable global to avoid
creating a lot of RTFs, and therefore avoid the whole "No more DTM IDs"
issue. It would make the process a lot more efficient.
Yes, it is an artificial transformation which should recreate the issue.
Some classes, such as Sort.java, still contain the old header, please
update them with the new ones such as that in DOM.java.
Did that.
The $Id section, such as the following, can all be removed, they were
from legacy repository, misleading since it implies the file was last
updated, in this case, in 2005:
20 /*
21 * $Id: Sort.java,v 1.2.4.1 2005/09/12 11:08:12 pvedula Exp $
22 */
Did that as well.
For the new test, it's probably better to add some kind of assertion in
the test, e.g. expected result, than failing on a broad Exception. What
if the test passes but the transform operation isn't because of the changes?
I've modified that part, asserting that the result matches a reference.
The test is also not sufficient. The release methods seem to be okay.
However, they don't seem to have been fully exercised in the test (only
simple RTs were created?). In that sense, the sample attached in
JDK-8150699 provided an opportunity to better verify the changes.
Yes, I had a hard time creating an artificial scenario which would reproduce
the issue and would also stress all places. I was rather running into
StackOverflows than out of DTM IDs. Eventually I managed to create something
but obviously not comprehensive enough. I also had some customer data which I
was eventually allowed to publish as testcase but the data was quite large and
the xsl very complex so the transformation would run very long. I have now
included the sample from JDK-8150699 into the test as well.
It would be good to add some javadoc or dev notes to the test. While
consolidating tests (into TransformerTest), please make sure
notes/javadoc are copied over or added.
I added a sentence of documentation for my testcase. For the consolidated ones
I now copied what was there and added a dummy summary text for the tests where
nothing was existing before.
This is the new webrev:
http://cr.openjdk.java.net/~clanger/webrevs/8150704.2/
I just ran the tests out of javax/xml/jaxp/unittest/transform. Maybe you will
want to do some more testing before pushing, e.g. JCK.
Let me know if I should do some further adaptions.
Thanks
Christoph
