You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by Christopher Schultz <ch...@christopherschultz.net> on 2020/12/14 16:01:16 UTC
Limiting VelocityLayoutServlet buffer size
All,
We recently suffered an OOME in production because we had a huge SQL
query return a log of data which, in turn, generated even more HTML
output for a particular screen.
We got an OOME with ASTNode.render trying to call StringWriter.write
(which calls StringBuffer's auto-re-sizing) because the output string
simply grew too much.
We are of course fixing the SQL query to make sure we limit the amount
of data that can come back, but we'd also like to make sure that
Velocity doesn't trigger an OOME like this if possible.
Is there a setting for limiting the output buffer size? I haven't yet
found one in the documentation, so I'm starting to look in the code.
Thanks,
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: Limiting VelocityLayoutServlet buffer size
Posted by Nathan Bubna <nb...@gmail.com>.
On Fri, Dec 18, 2020 at 3:01 PM Christopher Schultz <
chris@christopherschultz.net> wrote:
> Nathan,
>
> On 12/16/20 12:43, Nathan Bubna wrote:
> > In general, i think the overhead from StringWriter is probably pretty
> minimal.
> >
> > Before worrying about it, I'd want to see some profiling that showed it
> was a problem.
>
...
> In Java 8, StringBuilder wins by a factor of 10. In Java 15, it's a
> factor of ~20.
>
...
That's impressive.
> The question is whether or not it will make a difference when using a
> StringWriter
> versus a proposed StringBuilderWriter when the thing that's likely to
> take the most time is the many re-sizes of the internal char[] array
> that both use.
>
Yup. Impressive gains in a focused area don't give the full picture.
...
> Perhaps starting with a larger-than-default buffer for those will also
> help. (The default size of a StringWriter -- which is what
> VelocityLayoutServlet uses - is a mere 16 characters). I find it
> unlikely that such a small buffer would be practical for most Velocity
> workloads. Maybe we can improve performance a bit by specifying a larger
> default buffer/builder size to avoid some of the initial buffer-resizes
> that will inevitably occur.
>
Heh. I didn't realize StringWriter was so small by default. Sounds like a
promising optimization. 16 chars is obviously ludicrously small for
templating.
> Our dev resources these days are limited and things are pretty
> > stable, so i'm wary of taking on optimizations that increase our
> > complexity.
>
Honestly, the changes you are talking about are not that complex. I'm just
overwhelmed. This seems overcautious of me to bother saying in hindsight.
But you can probably see by how long i've let Velocity emails languish that
i just don't have useful amounts of time for it myself.
Re: Limiting VelocityLayoutServlet buffer size
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Nathan,
On 12/16/20 12:43, Nathan Bubna wrote:
> I don't honestly know much about JDK licensing, so i'll leave that to
> you or others. In general, i think the overhead from StringWriter is
> probably pretty minimal.
>
> Before worrying about it, I'd want to see some profiling that showed
> it was a problem.
I did a quick test of StringBuffer vs StringBuilder, trying to remove
everything I could from the test that could affect it other than the
obvious slight difference in synchronization strategy between the two.
In Java 8, StringBuilder wins by a factor of 10. In Java 15, it's a
factor of ~20.
Feel free to check my methodology. I'm attaching the source to my test
at the end of this post.
I'm assuming that StringWriter just being a thin wrapper around
StringBuffer is going to exhibit similar performance. The question is
whether or not it will make a difference when using a StringWriter
versus a proposed StringBuilderWriter when the thing that's likely to
take the most time is the many re-sizes of the internal char[] array
that both use.
I can build a new StringBuilderWriter and benchmark the two against each
other. (Actually, commons-io has a StringBuilderWriter since 2015, so I
can borrow that one for a performance-test).
Perhaps starting with a larger-than-default buffer for those will also
help. (The default size of a StringWriter -- which is what
VelocityLayoutServlet uses - is a mere 16 characters). I find it
unlikely that such a small buffer would be practical for most Velocity
workloads. Maybe we can improve performance a bit by specifying a larger
default buffer/builder size to avoid some of the initial buffer-resizes
that will inevitably occur.
> Our dev resources these days are limited and things are pretty
> stable, so i'm wary of taking on optimizations that increase our
> complexity.
Understood.
Thanks,
-chris
=== CUT ===
public class StringBuXtest {
public static void main(String[] args) {
int size = 100000;
int iterations = 5000;
StringBuffer buffer = new StringBuffer(size);
StringBuilder builder = new StringBuilder(size);
// Warm up
exercise(buffer, 10, size);
exercise(builder, 10, size);
exercise(buffer, 10, size);
exercise(builder, 10, size);
exercise(buffer, 10, size);
exercise(builder, 10, size);
long elapsed;
elapsed = System.currentTimeMillis();
exercise(buffer, iterations, size);
elapsed = System.currentTimeMillis() - elapsed;
System.out.println("StringBuffer: " + iterations + " iterations
in " + elapsed + "ms");
elapsed = System.currentTimeMillis();
exercise(builder, iterations, size);
elapsed = System.currentTimeMillis() - elapsed;
System.out.println("StringBuilder: " + iterations + "
iterations in " + elapsed + "ms");
}
public static void exercise(StringBuilder sb, int iterations, int
size) {
for(int i=0; i<iterations; ++i) {
sb.setLength(0);
for(int j=0; j < size; ++j) {
sb.append('a');
}
}
}
public static void exercise(StringBuffer sb, int iterations, int
size) {
for(int i=0; i<iterations; ++i) {
sb.setLength(0);
for(int j=0; j < size; ++j) {
sb.append('a');
}
}
}
}
=== CUT ===
> On Mon, Dec 14, 2020 at 8:38 AM Christopher Schultz <
> chris@christopherschultz.net> wrote:
>
>> Nathan,
>>
>> On 12/14/20 11:15, Nathan Bubna wrote:
>>> I'm pretty sure we don't have a setting like that.
>>
>> Yep, I just got into the code and was writing a patch for it:
>>
>> L265
>> protected void mergeTemplate(Template template, Context context,
>> HttpServletResponse response)
>> throws IOException
>> {
>> //
>> // this section is based on Tim Colson's "two pass render"
>> //
>> // Render the screen content
>> StringWriter sw = new StringWriter();
>> template.merge(context, sw);
>> // Add the resulting content to the context
>> context.put(KEY_SCREEN_CONTENT, sw.toString());
>>
>>
>> I have a LimitedStringWriter class that can be swapped-in for
>> StringWriter, but I'm having trouble building the JAR files due to an
>> unrelated build issue. I'll post about that separately.
>>
>> If I'm going to implement a limited StringWriter class, I'm wondering if
>> we shouldn't also take the opportunity to improve performance by using a
>> StringBuilder internally instead of a StringBuffer (which is what
>> java.io.StringWriter uses internally). Since this isn't expected to be
>> multi-threaded, the overhead of synchronization on StringWriter's
>> StringBuffer can be avoided. That would essentially require writing a
>> complete clone of StringWriter, which, while not complicated, might
>> possibly violate JDK licensing since I have recently read the code for
>> StringWriter in order to build the subclass. :(
>>
>> I didn't see anything in commons-lang or similar existing dependencies
>> that might fit the billing, so I have left it alone for now.
>>
>> -chris
>>
>>> On Mon, Dec 14, 2020 at 8:01 AM Christopher Schultz <
>>> chris@christopherschultz.net> wrote:
>>>
>>>> All,
>>>>
>>>> We recently suffered an OOME in production because we had a huge SQL
>>>> query return a log of data which, in turn, generated even more HTML
>>>> output for a particular screen.
>>>>
>>>> We got an OOME with ASTNode.render trying to call StringWriter.write
>>>> (which calls StringBuffer's auto-re-sizing) because the output string
>>>> simply grew too much.
>>>>
>>>> We are of course fixing the SQL query to make sure we limit the amount
>>>> of data that can come back, but we'd also like to make sure that
>>>> Velocity doesn't trigger an OOME like this if possible.
>>>>
>>>> Is there a setting for limiting the output buffer size? I haven't yet
>>>> found one in the documentation, so I'm starting to look in the code.
>>>>
>>>> Thanks,
>>>> -chris
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
>>>> For additional commands, e-mail: dev-help@velocity.apache.org
>>>>
>>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
>> For additional commands, e-mail: dev-help@velocity.apache.org
>>
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: Limiting VelocityLayoutServlet buffer size
Posted by Nathan Bubna <nb...@gmail.com>.
I don't honestly know much about JDK licensing, so i'll leave that to you
or others. In general, i think the overhead from StringWriter is probably
pretty minimal. Before worrying about it, i'd want to see some profiling
that showed it was a problem. Our dev resources these days are limited and
things are pretty stable, so i'm wary of taking on optimizations that
increase our complexity.
On Mon, Dec 14, 2020 at 8:38 AM Christopher Schultz <
chris@christopherschultz.net> wrote:
> Nathan,
>
> On 12/14/20 11:15, Nathan Bubna wrote:
> > I'm pretty sure we don't have a setting like that.
>
> Yep, I just got into the code and was writing a patch for it:
>
> L265
> protected void mergeTemplate(Template template, Context context,
> HttpServletResponse response)
> throws IOException
> {
> //
> // this section is based on Tim Colson's "two pass render"
> //
> // Render the screen content
> StringWriter sw = new StringWriter();
> template.merge(context, sw);
> // Add the resulting content to the context
> context.put(KEY_SCREEN_CONTENT, sw.toString());
>
>
> I have a LimitedStringWriter class that can be swapped-in for
> StringWriter, but I'm having trouble building the JAR files due to an
> unrelated build issue. I'll post about that separately.
>
> If I'm going to implement a limited StringWriter class, I'm wondering if
> we shouldn't also take the opportunity to improve performance by using a
> StringBuilder internally instead of a StringBuffer (which is what
> java.io.StringWriter uses internally). Since this isn't expected to be
> multi-threaded, the overhead of synchronization on StringWriter's
> StringBuffer can be avoided. That would essentially require writing a
> complete clone of StringWriter, which, while not complicated, might
> possibly violate JDK licensing since I have recently read the code for
> StringWriter in order to build the subclass. :(
>
> I didn't see anything in commons-lang or similar existing dependencies
> that might fit the billing, so I have left it alone for now.
>
> -chris
>
> > On Mon, Dec 14, 2020 at 8:01 AM Christopher Schultz <
> > chris@christopherschultz.net> wrote:
> >
> >> All,
> >>
> >> We recently suffered an OOME in production because we had a huge SQL
> >> query return a log of data which, in turn, generated even more HTML
> >> output for a particular screen.
> >>
> >> We got an OOME with ASTNode.render trying to call StringWriter.write
> >> (which calls StringBuffer's auto-re-sizing) because the output string
> >> simply grew too much.
> >>
> >> We are of course fixing the SQL query to make sure we limit the amount
> >> of data that can come back, but we'd also like to make sure that
> >> Velocity doesn't trigger an OOME like this if possible.
> >>
> >> Is there a setting for limiting the output buffer size? I haven't yet
> >> found one in the documentation, so I'm starting to look in the code.
> >>
> >> Thanks,
> >> -chris
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> >> For additional commands, e-mail: dev-help@velocity.apache.org
> >>
> >>
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
Re: Limiting VelocityLayoutServlet buffer size
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Nathan,
On 12/14/20 11:15, Nathan Bubna wrote:
> I'm pretty sure we don't have a setting like that.
Yep, I just got into the code and was writing a patch for it:
L265
protected void mergeTemplate(Template template, Context context,
HttpServletResponse response)
throws IOException
{
//
// this section is based on Tim Colson's "two pass render"
//
// Render the screen content
StringWriter sw = new StringWriter();
template.merge(context, sw);
// Add the resulting content to the context
context.put(KEY_SCREEN_CONTENT, sw.toString());
I have a LimitedStringWriter class that can be swapped-in for
StringWriter, but I'm having trouble building the JAR files due to an
unrelated build issue. I'll post about that separately.
If I'm going to implement a limited StringWriter class, I'm wondering if
we shouldn't also take the opportunity to improve performance by using a
StringBuilder internally instead of a StringBuffer (which is what
java.io.StringWriter uses internally). Since this isn't expected to be
multi-threaded, the overhead of synchronization on StringWriter's
StringBuffer can be avoided. That would essentially require writing a
complete clone of StringWriter, which, while not complicated, might
possibly violate JDK licensing since I have recently read the code for
StringWriter in order to build the subclass. :(
I didn't see anything in commons-lang or similar existing dependencies
that might fit the billing, so I have left it alone for now.
-chris
> On Mon, Dec 14, 2020 at 8:01 AM Christopher Schultz <
> chris@christopherschultz.net> wrote:
>
>> All,
>>
>> We recently suffered an OOME in production because we had a huge SQL
>> query return a log of data which, in turn, generated even more HTML
>> output for a particular screen.
>>
>> We got an OOME with ASTNode.render trying to call StringWriter.write
>> (which calls StringBuffer's auto-re-sizing) because the output string
>> simply grew too much.
>>
>> We are of course fixing the SQL query to make sure we limit the amount
>> of data that can come back, but we'd also like to make sure that
>> Velocity doesn't trigger an OOME like this if possible.
>>
>> Is there a setting for limiting the output buffer size? I haven't yet
>> found one in the documentation, so I'm starting to look in the code.
>>
>> Thanks,
>> -chris
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
>> For additional commands, e-mail: dev-help@velocity.apache.org
>>
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: Limiting VelocityLayoutServlet buffer size
Posted by Nathan Bubna <nb...@gmail.com>.
I'm pretty sure we don't have a setting like that.
On Mon, Dec 14, 2020 at 8:01 AM Christopher Schultz <
chris@christopherschultz.net> wrote:
> All,
>
> We recently suffered an OOME in production because we had a huge SQL
> query return a log of data which, in turn, generated even more HTML
> output for a particular screen.
>
> We got an OOME with ASTNode.render trying to call StringWriter.write
> (which calls StringBuffer's auto-re-sizing) because the output string
> simply grew too much.
>
> We are of course fixing the SQL query to make sure we limit the amount
> of data that can come back, but we'd also like to make sure that
> Velocity doesn't trigger an OOME like this if possible.
>
> Is there a setting for limiting the output buffer size? I haven't yet
> found one in the documentation, so I'm starting to look in the code.
>
> Thanks,
> -chris
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>