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

Severe performance regression (to 1.x) in certain cases #112

Closed
letmaik opened this issue Mar 31, 2013 · 8 comments
Closed

Severe performance regression (to 1.x) in certain cases #112

letmaik opened this issue Mar 31, 2013 · 8 comments

Comments

@letmaik
Copy link

@letmaik letmaik commented Mar 31, 2013

I'm a contributor to Ardor3D where jdom is used for handling a certain format of 3D models (Collada).

Recently I upgraded it from jdom 1.x to 2.x and today another contributor (@ricardolpd) noticed a big perfomance impact when loading one of his 4meg (xml) models. The slowdown happens when calling SAXBuilder.build(..).

I profiled everything using VisualVM and found the problem in org.jdom2.input.sax.TextBuffer. First, some numbers obtained with simple System.currentTimeMillis() benchmarking of SAXBuilder.build(..):

jdom1: ~270ms
jdom2: ~46000ms

In jdom2, 60% of the time (which was the top hotspot when profiling) was spent in org.jdom2.internal.ArrayCopy.copyOf(). This method is used in TextBuffer.append(). In jdom1, the underlying array of the text buffer was resized using the following method:

    private void ensureCapacity(int csize) {
        int capacity = array.length;
        if (csize > capacity) {
            char[] old = array;
            int nsize = capacity;
            while (csize > nsize) {
                nsize += (capacity/2);
            }
            array = new char[nsize];
            System.arraycopy(old, 0, array, 0, arraySize);
        }
    }

which was called in TextBuffer.append(char[] source, int start, int count) as ensureCapacity(arraySize + count);.

In jdom2, the array is resized directly in the append method by:

        if ((count + arraySize) > array.length) {
            array = ArrayCopy.copyOf(array, count + arraySize);
        }

Can you spot the difference? In jdom2, the array is resized to exactly the requested size. In jdom1, it is resized with an additional amount of extra space (nsize += (capacity/2);). As the extra buffer space is missing now, it causes severely more array copies than in jdom1 and therefore having an impact on performance, in case where the xml consists of much character data.

My suggestion is to revert to the resizing behavior of jdom1. Is something speaking against that?

Also, the class javadoc of TextBuffer isn't current anymore. It is still related to the jdom1 version where a prefixString was used.

letmaik referenced this issue in Renanse/Ardor3D Mar 31, 2013
@rolfl
Copy link
Collaborator

@rolfl rolfl commented Apr 1, 2013

Hi. OK, so I can't figure out why I changed the algorithm. It's a problem, I agree. Looking in to it some more and I will get back to you.

@rolfl
Copy link
Collaborator

@rolfl rolfl commented Apr 1, 2013

Hi, I have committed a fix to a new branch jdom-2.0.x which will be for the fixes coming to JDOM 2.0.5 and later. The master branch has items that are for JDOM 2.1.x and later (not yet released).

I am reluctant to create a full 2.0.5 build for this fix. It affects any users who have text content > 1024 chars, so it's a possibe 'big deal', but also the results are not 'wrong', just slow..... Typically, for JDOM, I schedule a release for 2-weeks after a fix is made, so that if anything eles comes up, it can be included too. I can provide a 'hotfix' as a short-term fix, or you can wait a couple of weeks, is there a preference?

@rolfl rolfl closed this Apr 1, 2013
rolfl added a commit that referenced this issue Apr 1, 2013
@letmaik
Copy link
Author

@letmaik letmaik commented Apr 1, 2013

Thanks for the fix.

Regarding releases, it's probably fine if you use your regular 2-weeks schedule. After all, the transition to jdom2 in Ardor3D was done in the current snapshot releases, so there hasn't been an official release with jdom2 yet.

@letmaik
Copy link
Author

@letmaik letmaik commented Apr 1, 2013

One more question though. The javadoc says "A non-public utility class similar to StringBuilder but optimized for XML parsing where the common case is that you get only one chunk of characters per text section." This optimization isn't existing anymore in jdom2. Why? In its current version, TextBuffer is essentially a StringBuilder, with a different growing strategy.

@rolfl
Copy link
Collaborator

@rolfl rolfl commented Apr 1, 2013

Hi. I have identified why I am confused about the TextBuffer. In a previous life (before I became the JDOM Maintainer) I did a 'rework' ( http://markmail.org/message/e2ljuk56mz337fqj ) of the code and I removed the TextBuffer class. In this life I was more conservative, it seems, and tried to improve it instead (and introduced this performance 'feature'). I think I will be conservative again on the 2.0.x branch, but will replace it entirely on the master (2.1.x branch).

@ricardolpd
Copy link

@ricardolpd ricardolpd commented Apr 12, 2013

@rolfl Hi will this patch be updated to maven? because currently Ardor3D uses maven to fetch the jar files.

@rolfl
Copy link
Collaborator

@rolfl rolfl commented Apr 12, 2013

Yes, it will be. A coupld of other issues have been raised in JDOM2 (nothing for 6 months, then 3 in two weeks....). They will all be released together 'real soon now'.

@ricardolpd
Copy link

@ricardolpd ricardolpd commented Apr 13, 2013

Thats excellent news,

Thanks once again for ur help.

Athou added a commit to Athou/commafeed that referenced this issue Jan 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.