Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4, 1.5, 1.6
    • Fix Version/s: 1.6
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Tested on FreeBSD 6.2-STABLE and Linux (Debian Etch) on i386.
      Java: JDK 1.5

      Description

      On some large templates (1-4MB) velocity gets very slow. I used JProfiler and found a lot of time is spent in VelocityCharStream.ExpandBuff. It is doing a lot of System.arraycopy.
      The problem is that the size of the buffer is increased linearly in stead of exponentialy.
      I have made a patch which doubles the size of the buffer in stead of incrementing it with the same value.
      In my tests and in JProfiler it is shown that a lot less time is spent in ExpandBuff.

      1. expandbuff-speedup.patch
        2 kB
        Ronald Klop
      2. expandbuff-speedup-reinit-fix.patch
        1 kB
        Jacob Mundt

        Activity

        Hide
        Nathan Bubna added a comment -

        Though i haven't hit the OOM without the patch myself, things continue to work well for me with the patch, and it looks sensible to me. I'm gonna go ahead and apply it based on Erron and Jarkko's reports.

        Show
        Nathan Bubna added a comment - Though i haven't hit the OOM without the patch myself, things continue to work well for me with the patch, and it looks sensible to me. I'm gonna go ahead and apply it based on Erron and Jarkko's reports.
        Hide
        Erron Austin added a comment -

        This patch has solved my OOM errors. I would rate this patch as critical. Before applying the patch I was able to cause OOM memory in my app by simply navigating around the application. With the patch, I'm able to put a good amount of load with very good performance. I applied this patch with trunk as of August 14, 2008.

        Show
        Erron Austin added a comment - This patch has solved my OOM errors. I would rate this patch as critical. Before applying the patch I was able to cause OOM memory in my app by simply navigating around the application. With the patch, I'm able to put a good amount of load with very good performance. I applied this patch with trunk as of August 14, 2008.
        Hide
        Nathan Bubna added a comment -

        ?? Jacob's fix doesn't look like a no-op to me. Not sure what you're [not] seeing, Will.

        Show
        Nathan Bubna added a comment - ?? Jacob's fix doesn't look like a no-op to me. Not sure what you're [not] seeing, Will.
        Hide
        Will Glass-Husain added a comment -

        really? It looked like a no-op to me. I'll take another look.

        Show
        Will Glass-Husain added a comment - really? It looked like a no-op to me. I'll take another look.
        Hide
        Jarkko Viinamäki added a comment -

        I can confirm that the current SVN head contains a bug which can be fixed by the patch Jacob submitted. The bug is critical and causes OutOfMemory error when a large template is parsed a couple of times (with caching off):

        Running org.apache.velocity.test.issues.RepeatedParsingTest
        iteration: 0
        iteration: 1
        java.lang.OutOfMemoryError: Java heap space
        at org.apache.velocity.runtime.parser.VelocityCharStream.ExpandBuff(VelocityCharStream.java:
        67)
        at org.apache.velocity.runtime.parser.VelocityCharStream.FillBuff(VelocityCharStream.java:13
        1)
        at org.apache.velocity.runtime.parser.VelocityCharStream.readChar(VelocityCharStream.java:24
        6)
        at org.apache.velocity.runtime.parser.ParserTokenManager.jjMoveNfa_3(ParserTokenManager.java
        :2160)

        Show
        Jarkko Viinamäki added a comment - I can confirm that the current SVN head contains a bug which can be fixed by the patch Jacob submitted. The bug is critical and causes OutOfMemory error when a large template is parsed a couple of times (with caching off): Running org.apache.velocity.test.issues.RepeatedParsingTest iteration: 0 iteration: 1 java.lang.OutOfMemoryError: Java heap space at org.apache.velocity.runtime.parser.VelocityCharStream.ExpandBuff(VelocityCharStream.java: 67) at org.apache.velocity.runtime.parser.VelocityCharStream.FillBuff(VelocityCharStream.java:13 1) at org.apache.velocity.runtime.parser.VelocityCharStream.readChar(VelocityCharStream.java:24 6) at org.apache.velocity.runtime.parser.ParserTokenManager.jjMoveNfa_3(ParserTokenManager.java :2160)
        Hide
        Nathan Bubna added a comment -

        Releases happen when someone takes the time to push one through. I don't know if i will really get to it amidst the busyness of summer, but i have been hoping to start pushing a 1.6 release process by summer's end. I'm also rather anxious to start using features in 1.6...

        Show
        Nathan Bubna added a comment - Releases happen when someone takes the time to push one through. I don't know if i will really get to it amidst the busyness of summer, but i have been hoping to start pushing a 1.6 release process by summer's end. I'm also rather anxious to start using features in 1.6...
        Hide
        Ronald Klop added a comment -

        Hi,

        Is there an idea about when this fix gets into a stable release?
        I see there are beta's of velocity tools, but not for the velocity engine.

        Ronald.

        Show
        Ronald Klop added a comment - Hi, Is there an idea about when this fix gets into a stable release? I see there are beta's of velocity tools, but not for the velocity engine. Ronald.
        Hide
        Will Glass-Husain added a comment -

        Hi –

        I took a look at this older issue. Jacob-- it looks like your patch doesn't accomplish anything. VelocityCharStream is always init''d with a buffer size of 4096 as the constructor and ReInit that's called doesn't specify a buffer size. Can you take another look?

        As a side note, there's a bug in the class here

        public void ReInit(java.io.InputStream dstream, int startline,
        int startcolumn, int buffersize)

        { ReInit(new java.io.InputStreamReader(dstream), startline, startcolumn, 4096); }

        should be

        public void ReInit(java.io.InputStream dstream, int startline,
        int startcolumn, int buffersize)

        { ReInit(new java.io.InputStreamReader(dstream), startline, startcolumn,buffersize); }

        I fixed this, but it's not really relevant since the constructor is never called.

        Show
        Will Glass-Husain added a comment - Hi – I took a look at this older issue. Jacob-- it looks like your patch doesn't accomplish anything. VelocityCharStream is always init''d with a buffer size of 4096 as the constructor and ReInit that's called doesn't specify a buffer size. Can you take another look? As a side note, there's a bug in the class here public void ReInit(java.io.InputStream dstream, int startline, int startcolumn, int buffersize) { ReInit(new java.io.InputStreamReader(dstream), startline, startcolumn, 4096); } should be public void ReInit(java.io.InputStream dstream, int startline, int startcolumn, int buffersize) { ReInit(new java.io.InputStreamReader(dstream), startline, startcolumn,buffersize); } I fixed this, but it's not really relevant since the constructor is never called.
        Hide
        Will Glass-Husain added a comment -

        reopening to prompt review of new patch. Thanks very much!

        Show
        Will Glass-Husain added a comment - reopening to prompt review of new patch. Thanks very much!
        Hide
        Jacob Mundt added a comment -

        The original patch doesn't reset the nextBufExpand increment when ReInit is called-so even after ReInit is called with a tiny buffer size, the nextBufExpand increment could still be very large-my simple test loop (parsing a 3MB template repeatedly) actually ran out of memory after a few iterations.

        Attached a patch which fixes this. I also changed the initial increment to be based on the buffer size, rather than always using 2048.

        Show
        Jacob Mundt added a comment - The original patch doesn't reset the nextBufExpand increment when ReInit is called- so even after ReInit is called with a tiny buffer size, the nextBufExpand increment could still be very large -my simple test loop (parsing a 3MB template repeatedly) actually ran out of memory after a few iterations. Attached a patch which fixes this. I also changed the initial increment to be based on the buffer size, rather than always using 2048.
        Hide
        Will Glass-Husain added a comment -

        Hey that's really fabulous. Thanks so much for finding this issue and fixing it. I'm sure future users with big templates will especially appreciate it.

        One item I note is that that there's no unit test for VelocityCharStream. If you felt like writing one and submitting it, that would be a nice addition to the test suite.

        Show
        Will Glass-Husain added a comment - Hey that's really fabulous. Thanks so much for finding this issue and fixing it. I'm sure future users with big templates will especially appreciate it. One item I note is that that there's no unit test for VelocityCharStream. If you felt like writing one and submitting it, that would be a nice addition to the test suite.
        Hide
        Claude Brisson added a comment -

        Thank you very much!
        Patch reviewed, looks perfect.
        Applied.

        Show
        Claude Brisson added a comment - Thank you very much! Patch reviewed, looks perfect. Applied.
        Hide
        Ronald Klop added a comment -

        I have made the patch so that it touches as few lines as possible.

        Show
        Ronald Klop added a comment - I have made the patch so that it touches as few lines as possible.

          People

          • Assignee:
            Unassigned
            Reporter:
            Ronald Klop
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development