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
