You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by Alex Fedotov <al...@kayak.com> on 2016/11/28 19:52:02 UTC
2.0 question
Hello All,
ContextAware interface in Velocity is not really workable in a
multi-threaded environment.
The handler is a singleton and setting the Context on the singleton from
multiple threads is not going to work well, short of using some form of
thread local.
Also the event handlers (such as insertion handler) are creating new
instance of executor for each inserted value which is inefficient.
Are you guys fixing any of this for 2.0?
Thanks,
Alex
Re: 2.0 question
Posted by Alex Fedotov <al...@kayak.com>.
I remembered one more thing:
I think we wanted to include template call stack and macro call stack into
the MethodInvocationException message. There was no easy way to do it
without installing a ContextAware MethodExceptionEventHandler because the
stack info is only available from the Context.
I guess this would be covered by passing the Context to all event handlers.
Alex
On Wed, Nov 30, 2016 at 11:26 AM, Alex Fedotov <al...@kayak.com> wrote:
> As far as event handlers and passing the RuntimeServices in
> initialization. There is no problem with that, except that the interface is
> optional. You end-up with the code like EventCartridge.initialize.
>
> May be the event handler interfaces like ReferenceInsertionEventHandler
> should just derive from RuntimeServicesAware. This way you can just call it
> unconditionally at creation time without any "instanceof" checks.
> In Java 8 "setRuntimeServices" could be a default method on the interface
> with a NOOP implementation.
>
> Alex
>
>
> On Wed, Nov 30, 2016 at 4:56 AM, Claude Brisson <cl...@renegat.net>
> wrote:
>
>> Thanks for the details.
>>
>> I agree that the Context could be passed along with other standard
>> arguments. It has been done this way for backward compatibility, but since
>> we go 2.0, we can improve it. And even keep B.C. by introducing new
>> interfaces while deprecating the old ones.
>>
>> It's about the fourth or fifth time that I prepare a release candidate,
>> but I think it's worth. Hopefully I'm now pretty used to it...
>>
>> On 29/11/2016 23:25, Alex Fedotov wrote:
>>
>> [...]
>>
>> It seems like there is a some unnecessary work being done initializing
>>> event cartridges, etc. and all of that just for the purpose of setting
>>> RuntimeServices and Context references on them.
>>>
>>
>> What's wrong with setting RuntimeServices at initialization?
>>
>> As far as creation of executors:
>>> I understand that it was convenient to create the method
>>> "iterateOverEventHandlers" and apply it for event handlers with different
>>> parameters, but the cost of that is creation of the new executor instance
>>> for every invocation.
>>> It would be more efficient to just have a utility that returns a combined
>>> list of handlers (if needed) instead of creating one iterator for each
>>> list
>>> (application and context). Then the callback invocation code could just
>>> walk the list and call the handlers.
>>>
>>
>> On the assumption that you have implemented it, did you observe any real
>> performance gain with this change alone? Because once again, since JDK 1.5
>> or 1.6, the allocation of small objects doesn't cost much more than a
>> function call. For instance, see :
>> http://www.ibm.com/developerworks/library/j-jtp09275/
>> (and yet, this article dates back to 2005...)
>>
>>
>> We have run into some other inefficient places. For example
>>> ASTStringLiteral is buffering the entire content in the StringWriter. It
>>> does not work very well for large templates.
>>> That code creates a writer which buffers up the whole thing, then does a
>>> toString() on it creating another copy. Then in some cases calls
>>> substring() that creates yet another copy.
>>>
>>
>> Oh, I never noticed that! That must be one of the very few parser node
>> classes I didn't review...
>> I agree it looks like very inefficient. I guess it has be done this way
>> so that the node rendering is all or nothing, in case there's a throw
>> exception. But considering the allocation impact, it cannot stand.
>>
>> I can dig up a few other things we fixed in our version if you guys are
>>> interested.
>>>
>>
>> We are of course interested. But should you submit any code, you have to
>> also submit a license agreement (see https://www.apache.org/license
>> s/#submitting ) so that we can use it.
>>
>>
>>
>> Claude
>>
>>
>
Re: 2.0 question
Posted by Alex Fedotov <al...@kayak.com>.
As far as event handlers and passing the RuntimeServices in initialization.
There is no problem with that, except that the interface is optional. You
end-up with the code like EventCartridge.initialize.
May be the event handler interfaces like ReferenceInsertionEventHandler
should just derive from RuntimeServicesAware. This way you can just call it
unconditionally at creation time without any "instanceof" checks.
In Java 8 "setRuntimeServices" could be a default method on the interface
with a NOOP implementation.
Alex
On Wed, Nov 30, 2016 at 4:56 AM, Claude Brisson <cl...@renegat.net> wrote:
> Thanks for the details.
>
> I agree that the Context could be passed along with other standard
> arguments. It has been done this way for backward compatibility, but since
> we go 2.0, we can improve it. And even keep B.C. by introducing new
> interfaces while deprecating the old ones.
>
> It's about the fourth or fifth time that I prepare a release candidate,
> but I think it's worth. Hopefully I'm now pretty used to it...
>
> On 29/11/2016 23:25, Alex Fedotov wrote:
>
> [...]
>
> It seems like there is a some unnecessary work being done initializing
>> event cartridges, etc. and all of that just for the purpose of setting
>> RuntimeServices and Context references on them.
>>
>
> What's wrong with setting RuntimeServices at initialization?
>
> As far as creation of executors:
>> I understand that it was convenient to create the method
>> "iterateOverEventHandlers" and apply it for event handlers with different
>> parameters, but the cost of that is creation of the new executor instance
>> for every invocation.
>> It would be more efficient to just have a utility that returns a combined
>> list of handlers (if needed) instead of creating one iterator for each
>> list
>> (application and context). Then the callback invocation code could just
>> walk the list and call the handlers.
>>
>
> On the assumption that you have implemented it, did you observe any real
> performance gain with this change alone? Because once again, since JDK 1.5
> or 1.6, the allocation of small objects doesn't cost much more than a
> function call. For instance, see :
> http://www.ibm.com/developerworks/library/j-jtp09275/
> (and yet, this article dates back to 2005...)
>
>
> We have run into some other inefficient places. For example
>> ASTStringLiteral is buffering the entire content in the StringWriter. It
>> does not work very well for large templates.
>> That code creates a writer which buffers up the whole thing, then does a
>> toString() on it creating another copy. Then in some cases calls
>> substring() that creates yet another copy.
>>
>
> Oh, I never noticed that! That must be one of the very few parser node
> classes I didn't review...
> I agree it looks like very inefficient. I guess it has be done this way so
> that the node rendering is all or nothing, in case there's a throw
> exception. But considering the allocation impact, it cannot stand.
>
> I can dig up a few other things we fixed in our version if you guys are
>> interested.
>>
>
> We are of course interested. But should you submit any code, you have to
> also submit a license agreement (see https://www.apache.org/license
> s/#submitting ) so that we can use it.
>
>
>
> Claude
>
>
Re: 2.0 question
Posted by Claude Brisson <cl...@renegat.net>.
Thanks for the details.
I agree that the Context could be passed along with other standard
arguments. It has been done this way for backward compatibility, but
since we go 2.0, we can improve it. And even keep B.C. by introducing
new interfaces while deprecating the old ones.
It's about the fourth or fifth time that I prepare a release candidate,
but I think it's worth. Hopefully I'm now pretty used to it...
On 29/11/2016 23:25, Alex Fedotov wrote:
[...]
> It seems like there is a some unnecessary work being done initializing
> event cartridges, etc. and all of that just for the purpose of setting
> RuntimeServices and Context references on them.
What's wrong with setting RuntimeServices at initialization?
> As far as creation of executors:
> I understand that it was convenient to create the method
> "iterateOverEventHandlers" and apply it for event handlers with different
> parameters, but the cost of that is creation of the new executor instance
> for every invocation.
> It would be more efficient to just have a utility that returns a combined
> list of handlers (if needed) instead of creating one iterator for each list
> (application and context). Then the callback invocation code could just
> walk the list and call the handlers.
On the assumption that you have implemented it, did you observe any real
performance gain with this change alone? Because once again, since JDK
1.5 or 1.6, the allocation of small objects doesn't cost much more than
a function call. For instance, see :
http://www.ibm.com/developerworks/library/j-jtp09275/
(and yet, this article dates back to 2005...)
> We have run into some other inefficient places. For example
> ASTStringLiteral is buffering the entire content in the StringWriter. It
> does not work very well for large templates.
> That code creates a writer which buffers up the whole thing, then does a
> toString() on it creating another copy. Then in some cases calls
> substring() that creates yet another copy.
Oh, I never noticed that! That must be one of the very few parser node
classes I didn't review...
I agree it looks like very inefficient. I guess it has be done this way
so that the node rendering is all or nothing, in case there's a throw
exception. But considering the allocation impact, it cannot stand.
> I can dig up a few other things we fixed in our version if you guys are
> interested.
We are of course interested. But should you submit any code, you have to
also submit a license agreement (see
https://www.apache.org/licenses/#submitting ) so that we can use it.
Claude
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: 2.0 question
Posted by Claude Brisson <cl...@renegat.net>.
It is not so hard to differenciate the cases. You just have to look to
the callstack, and it will exactly give you the parents chain.
If your ASTReference is directly in ASTprocess, or inside an ASTBlock,
it is being rendered. If your ASTReference is somewhere underneath an
ASTExpression inside an ASTIfStatement or ASTElseIfStatement, without
any ASTBlock in between, it is being tested.
Claude
On 15/12/2016 19:31, Alex Fedotov wrote:
> One more item that comes to mind with event handlers:
>
> In the current interface they do not receive the AST node, just some fields
> from the node.
> In ReferenceInsertionEventHandler it makes very hard to differentiate
> between the insert into the template output as opposed to some string
> literal.
>
> For example:
>
> <div #if("$!blah" != "") attribute="$!blah">
>
> In this case the ReferenceInsertionEventHandler is called two times. Once
> in the context of beining inserted into the ASTStringLiteral "$!blah" and
> the second time when inserted into the template (attribute="$!blah").
> Currently it's difficult to differentiate these cases. If the node itself
> would be passed to the handler one could navigate through the parent chain
> and detect if the insertion is inside of the ASTStringLiteral.
>
>
>
>
>
>
>
> On Wed, Dec 14, 2016 at 4:08 AM, Claude Brisson <cl...@renegat.net> wrote:
>
>> It's the VelocityView from the tools subproject, which uses a pool of
>> VelocityWriters. The engine itself doesn't know which writer is given as
>> argument.
>>
>> Yes, it's *possible* to do it. But it also implies breaking backward
>> compatibility for the o.a.v.io.Filter API, all for the sake of
>> ASTStringLiteral's interpolation performance for big interpolated templates
>> (there is no other concerned case in standard use).
>>
>> So the trade-off is not worth. Plus, even if we try to be as fast as we
>> can with the current code paradigms, the whole engine would have to be
>> rewritten using nio buffers and file mapping to address a real performance
>> gain. This is not the purpose of the 2.0 version.
>>
>> Claude
>>
>>
>>
>> On 13/12/2016 16:01, Alex Fedotov wrote:
>>
>>> I guess it depends what Writer is used.
>>> StringBuilderWriter.append(CharSequence) is overloaded.
>>>
>>> I thought that Velocity is using a pool of VelocityWriter instances, not
>>> an
>>> OutputStreamWriter. If that is so you can overload append(CharSequence)
>>> as
>>> needed in VelocityWriter.
>>>
>>> JDK default implementation is very poor (creating potentially two copies
>>> of
>>> data in subSequence and toString).
>>>
>>> public Writer append(CharSequence csq, int start, int end) throws
>>> IOException {
>>> CharSequence cs = (csq == null ? "null" : csq);
>>> write(cs.subSequence(start, end).toString());
>>> return this;
>>> }
>>>
>>>
>>> On Tue, Dec 13, 2016 at 8:48 AM, Claude Brisson <cl...@renegat.net>
>>> wrote:
>>>
>>> Reducing useless synchronizations by using a StringBuilderWriter was easy.
>>>> But about using CharSequences instead of Strings, after a quick look, it
>>>> doesn't look so promising: did you know that Writer.append(CharSequence)
>>>> does call Writer.write(sequence.toString()), which will itself use
>>>> string.getChars(), at leats in its default version?!
>>>>
>>>> The most frightening about this code in java.io.Writer is that whenever
>>>> you pass big strings to it, it will each time allocate big buffers
>>>> instead
>>>> of, for instance, copy small portions to output inside a loop. Time
>>>> needed
>>>> for memory copy is often negligible, whereas time needed to allocate big
>>>> chunks of memory is rather annoying. Twice as annoying when done twice.
>>>>
>>>> There is no point in trying to avoid the toString() call in Velocity if
>>>> the i/o lower layer will call it anyway.
>>>>
>>>> Claude
>>>>
>>>>
>>>>
>>>> On 12/12/2016 18:15, Alex Fedotov wrote:
>>>>
>>>> I don't know if something like this is in scope fro 2.0, but for more
>>>>> advanced manipulation of AST it would be helpful if creation of all AST
>>>>> nodes would be delegated to some kind of configurable factory class.
>>>>>
>>>>> This way if I wanted to replace ASTStringLiteral with a subclass of
>>>>> ASTStringLiteral for optimization purposes (say use a chunked buffer, or
>>>>> something similar) I could then setup my own factory and create an
>>>>> instance
>>>>> of a subclass.
>>>>>
>>>>> Alex
>>>>>
>>>>>
>>>>> On Mon, Dec 12, 2016 at 11:52 AM, Claude Brisson <cl...@renegat.net>
>>>>> wrote:
>>>>>
>>>>> Yes, I also thought of that. But a particular care must be taken in
>>>>>
>>>>>> upstream code that may expect to encounter String values and may
>>>>>> otherwise
>>>>>> call toString(), voiding the benefice. Worth looking at, though ; it
>>>>>> must
>>>>>> not be too difficult to have this code take CharSequence values into
>>>>>> account.
>>>>>>
>>>>>> Claude
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 12/12/2016 15:58, Alex Fedotov wrote:
>>>>>>
>>>>>> I am not sure if it's possible to assume that ASTStringLiteral.value()
>>>>>>
>>>>>>> returns a CharSequence and not a String. If that was the case you
>>>>>>> could
>>>>>>> just return the StringBuilder directly (StringBuilder implements
>>>>>>> CharSequence) without calling StringBuilder.toString(). This would
>>>>>>> avoid
>>>>>>> making one more copy of the data.
>>>>>>>
>>>>>>> Same may apply in other places.
>>>>>>>
>>>>>>> Alex
>>>>>>>
>>>>>>>
>>>>>>> On Sun, Dec 11, 2016 at 12:28 PM, Claude Brisson <cl...@renegat.net>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Thanks. I'll review those StringBuffer uses.
>>>>>>>
>>>>>>> For the ASTStringLiteral, I see there's already a StringBuilderWriter
>>>>>>>> in
>>>>>>>> commons-io that I may borrow...
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/12/2016 17:07, Alex Fedotov wrote:
>>>>>>>>
>>>>>>>> Great thank you.
>>>>>>>>
>>>>>>>> Actually I would recommend removing the StringWriter. StringWriter is
>>>>>>>>> using
>>>>>>>>> StringBuffer inside which is synchronized.
>>>>>>>>> In case of ASTStringLiteral there is no need to synchronize
>>>>>>>>> anything.
>>>>>>>>> There
>>>>>>>>> must be a Writer implementation based on StringBuilder instead.
>>>>>>>>>
>>>>>>>>> I have a run a search on the latest 2.0 code and it seems like there
>>>>>>>>> is
>>>>>>>>> a
>>>>>>>>> number of places where StringBuffer is used.
>>>>>>>>> Unless synchronization is required those usages should really be
>>>>>>>>> replaced
>>>>>>>>> with StringBuilder(s).
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ============================================================
>>>>>>>>> =================
>>>>>>>>>
>>>>>>>>> As far our usage of large string literal it was in a legacy
>>>>>>>>> framework
>>>>>>>>> that
>>>>>>>>> we had.
>>>>>>>>> It was creating a system of macros to render "tiles" on a page.
>>>>>>>>>
>>>>>>>>> Similar to this syntax (not real):
>>>>>>>>>
>>>>>>>>> @#PageSection(param1, param2....)
>>>>>>>>> @#SubSection("tileName1")
>>>>>>>>> some html
>>>>>>>>> #end
>>>>>>>>> @#SubSection("tileName2")
>>>>>>>>> some other html
>>>>>>>>> #end
>>>>>>>>> #SubSection("tileName3", "/blah/blah/section.vtl")
>>>>>>>>> #end
>>>>>>>>>
>>>>>>>>> Velocity macros with body allow only one body block (special
>>>>>>>>> parameter
>>>>>>>>> really).
>>>>>>>>> We needed multiple named body blocks which we could render in the
>>>>>>>>> correct
>>>>>>>>> spots inside of the macro.
>>>>>>>>>
>>>>>>>>> We are no longer using this framework, but that is where we had
>>>>>>>>> large
>>>>>>>>> string literals appearing.
>>>>>>>>> I think we replaced the StringBuffer with some unsynchronized
>>>>>>>>> chunked
>>>>>>>>> implementation to avoid allocation of very large strings and
>>>>>>>>> unnecessary
>>>>>>>>> synchronzation.
>>>>>>>>>
>>>>>>>>> Alex
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Sun, Dec 11, 2016 at 7:18 AM, Claude Brisson <claude@renegat.net
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> FYI, we got rid of the Executor pattern in the events API, and we
>>>>>>>>> now
>>>>>>>>>
>>>>>>>>> always provide the current Context when calling handlers.
>>>>>>>>>
>>>>>>>>>> On 29/11/2016 23:25, Alex Fedotov wrote:
>>>>>>>>>> [...]
>>>>>>>>>>
>>>>>>>>>> We have run into some other inefficient places. For example
>>>>>>>>>>
>>>>>>>>>> ASTStringLiteral is buffering the entire content in the
>>>>>>>>>> StringWriter.
>>>>>>>>>>
>>>>>>>>>>> It
>>>>>>>>>>> does not work very well for large templates.
>>>>>>>>>>> That code creates a writer which buffers up the whole thing, then
>>>>>>>>>>> does a
>>>>>>>>>>> toString() on it creating another copy. Then in some cases calls
>>>>>>>>>>> substring() that creates yet another copy.
>>>>>>>>>>>
>>>>>>>>>>> The substring belonged to an old workaround, it has been removed.
>>>>>>>>>>>
>>>>>>>>>>> The StringWriter buffering is only done when interpolation is
>>>>>>>>>>> needed
>>>>>>>>>>>
>>>>>>>>>> (that
>>>>>>>>>> is, when the string literal is enclosed in double quotes and has
>>>>>>>>>> some $
>>>>>>>>>> or
>>>>>>>>>> # inside). There could be some tricky ways of introducing some kind
>>>>>>>>>> of
>>>>>>>>>> lazy
>>>>>>>>>> evaluation, that would resort to using the provided final writer
>>>>>>>>>> when
>>>>>>>>>> the
>>>>>>>>>> value is meant to be rendered or to a buffered writer otherwise.
>>>>>>>>>> But
>>>>>>>>>> I'm
>>>>>>>>>> not sure it is worth it, because it looks rather fragile and
>>>>>>>>>> convoluted.
>>>>>>>>>> Plus, you can generally avoid having big string literals. I don't
>>>>>>>>>> know
>>>>>>>>>> about your use case, but why can't something as:
>>>>>>>>>>
>>>>>>>>>> #set($foo = "#parse('big.vtl')") $foo
>>>>>>>>>>
>>>>>>>>>> be rewritten as:
>>>>>>>>>>
>>>>>>>>>> #parse('big.vtl')
>>>>>>>>>>
>>>>>>>>>> to avoid the buffering?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Claude
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> ------------------------------------------------------------
>>>>>>>>>> ---------
>>>>>>>>>>
>>>>>>>>>> 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
>>>>
>>>>
>>>>
>> ---------------------------------------------------------------------
>> 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: 2.0 question
Posted by Alex Fedotov <al...@kayak.com>.
One more item that comes to mind with event handlers:
In the current interface they do not receive the AST node, just some fields
from the node.
In ReferenceInsertionEventHandler it makes very hard to differentiate
between the insert into the template output as opposed to some string
literal.
For example:
<div #if("$!blah" != "") attribute="$!blah">
In this case the ReferenceInsertionEventHandler is called two times. Once
in the context of beining inserted into the ASTStringLiteral "$!blah" and
the second time when inserted into the template (attribute="$!blah").
Currently it's difficult to differentiate these cases. If the node itself
would be passed to the handler one could navigate through the parent chain
and detect if the insertion is inside of the ASTStringLiteral.
On Wed, Dec 14, 2016 at 4:08 AM, Claude Brisson <cl...@renegat.net> wrote:
> It's the VelocityView from the tools subproject, which uses a pool of
> VelocityWriters. The engine itself doesn't know which writer is given as
> argument.
>
> Yes, it's *possible* to do it. But it also implies breaking backward
> compatibility for the o.a.v.io.Filter API, all for the sake of
> ASTStringLiteral's interpolation performance for big interpolated templates
> (there is no other concerned case in standard use).
>
> So the trade-off is not worth. Plus, even if we try to be as fast as we
> can with the current code paradigms, the whole engine would have to be
> rewritten using nio buffers and file mapping to address a real performance
> gain. This is not the purpose of the 2.0 version.
>
> Claude
>
>
>
> On 13/12/2016 16:01, Alex Fedotov wrote:
>
>> I guess it depends what Writer is used.
>> StringBuilderWriter.append(CharSequence) is overloaded.
>>
>> I thought that Velocity is using a pool of VelocityWriter instances, not
>> an
>> OutputStreamWriter. If that is so you can overload append(CharSequence)
>> as
>> needed in VelocityWriter.
>>
>> JDK default implementation is very poor (creating potentially two copies
>> of
>> data in subSequence and toString).
>>
>> public Writer append(CharSequence csq, int start, int end) throws
>> IOException {
>> CharSequence cs = (csq == null ? "null" : csq);
>> write(cs.subSequence(start, end).toString());
>> return this;
>> }
>>
>>
>> On Tue, Dec 13, 2016 at 8:48 AM, Claude Brisson <cl...@renegat.net>
>> wrote:
>>
>> Reducing useless synchronizations by using a StringBuilderWriter was easy.
>>>
>>> But about using CharSequences instead of Strings, after a quick look, it
>>> doesn't look so promising: did you know that Writer.append(CharSequence)
>>> does call Writer.write(sequence.toString()), which will itself use
>>> string.getChars(), at leats in its default version?!
>>>
>>> The most frightening about this code in java.io.Writer is that whenever
>>> you pass big strings to it, it will each time allocate big buffers
>>> instead
>>> of, for instance, copy small portions to output inside a loop. Time
>>> needed
>>> for memory copy is often negligible, whereas time needed to allocate big
>>> chunks of memory is rather annoying. Twice as annoying when done twice.
>>>
>>> There is no point in trying to avoid the toString() call in Velocity if
>>> the i/o lower layer will call it anyway.
>>>
>>> Claude
>>>
>>>
>>>
>>> On 12/12/2016 18:15, Alex Fedotov wrote:
>>>
>>> I don't know if something like this is in scope fro 2.0, but for more
>>>> advanced manipulation of AST it would be helpful if creation of all AST
>>>> nodes would be delegated to some kind of configurable factory class.
>>>>
>>>> This way if I wanted to replace ASTStringLiteral with a subclass of
>>>> ASTStringLiteral for optimization purposes (say use a chunked buffer, or
>>>> something similar) I could then setup my own factory and create an
>>>> instance
>>>> of a subclass.
>>>>
>>>> Alex
>>>>
>>>>
>>>> On Mon, Dec 12, 2016 at 11:52 AM, Claude Brisson <cl...@renegat.net>
>>>> wrote:
>>>>
>>>> Yes, I also thought of that. But a particular care must be taken in
>>>>
>>>>> upstream code that may expect to encounter String values and may
>>>>> otherwise
>>>>> call toString(), voiding the benefice. Worth looking at, though ; it
>>>>> must
>>>>> not be too difficult to have this code take CharSequence values into
>>>>> account.
>>>>>
>>>>> Claude
>>>>>
>>>>>
>>>>>
>>>>> On 12/12/2016 15:58, Alex Fedotov wrote:
>>>>>
>>>>> I am not sure if it's possible to assume that ASTStringLiteral.value()
>>>>>
>>>>>> returns a CharSequence and not a String. If that was the case you
>>>>>> could
>>>>>> just return the StringBuilder directly (StringBuilder implements
>>>>>> CharSequence) without calling StringBuilder.toString(). This would
>>>>>> avoid
>>>>>> making one more copy of the data.
>>>>>>
>>>>>> Same may apply in other places.
>>>>>>
>>>>>> Alex
>>>>>>
>>>>>>
>>>>>> On Sun, Dec 11, 2016 at 12:28 PM, Claude Brisson <cl...@renegat.net>
>>>>>> wrote:
>>>>>>
>>>>>> Thanks. I'll review those StringBuffer uses.
>>>>>>
>>>>>> For the ASTStringLiteral, I see there's already a StringBuilderWriter
>>>>>>> in
>>>>>>> commons-io that I may borrow...
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 11/12/2016 17:07, Alex Fedotov wrote:
>>>>>>>
>>>>>>> Great thank you.
>>>>>>>
>>>>>>> Actually I would recommend removing the StringWriter. StringWriter is
>>>>>>>> using
>>>>>>>> StringBuffer inside which is synchronized.
>>>>>>>> In case of ASTStringLiteral there is no need to synchronize
>>>>>>>> anything.
>>>>>>>> There
>>>>>>>> must be a Writer implementation based on StringBuilder instead.
>>>>>>>>
>>>>>>>> I have a run a search on the latest 2.0 code and it seems like there
>>>>>>>> is
>>>>>>>> a
>>>>>>>> number of places where StringBuffer is used.
>>>>>>>> Unless synchronization is required those usages should really be
>>>>>>>> replaced
>>>>>>>> with StringBuilder(s).
>>>>>>>>
>>>>>>>>
>>>>>>>> ============================================================
>>>>>>>> =================
>>>>>>>>
>>>>>>>> As far our usage of large string literal it was in a legacy
>>>>>>>> framework
>>>>>>>> that
>>>>>>>> we had.
>>>>>>>> It was creating a system of macros to render "tiles" on a page.
>>>>>>>>
>>>>>>>> Similar to this syntax (not real):
>>>>>>>>
>>>>>>>> @#PageSection(param1, param2....)
>>>>>>>> @#SubSection("tileName1")
>>>>>>>> some html
>>>>>>>> #end
>>>>>>>> @#SubSection("tileName2")
>>>>>>>> some other html
>>>>>>>> #end
>>>>>>>> #SubSection("tileName3", "/blah/blah/section.vtl")
>>>>>>>> #end
>>>>>>>>
>>>>>>>> Velocity macros with body allow only one body block (special
>>>>>>>> parameter
>>>>>>>> really).
>>>>>>>> We needed multiple named body blocks which we could render in the
>>>>>>>> correct
>>>>>>>> spots inside of the macro.
>>>>>>>>
>>>>>>>> We are no longer using this framework, but that is where we had
>>>>>>>> large
>>>>>>>> string literals appearing.
>>>>>>>> I think we replaced the StringBuffer with some unsynchronized
>>>>>>>> chunked
>>>>>>>> implementation to avoid allocation of very large strings and
>>>>>>>> unnecessary
>>>>>>>> synchronzation.
>>>>>>>>
>>>>>>>> Alex
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sun, Dec 11, 2016 at 7:18 AM, Claude Brisson <claude@renegat.net
>>>>>>>> >
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> FYI, we got rid of the Executor pattern in the events API, and we
>>>>>>>> now
>>>>>>>>
>>>>>>>> always provide the current Context when calling handlers.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 29/11/2016 23:25, Alex Fedotov wrote:
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>> We have run into some other inefficient places. For example
>>>>>>>>>
>>>>>>>>> ASTStringLiteral is buffering the entire content in the
>>>>>>>>> StringWriter.
>>>>>>>>>
>>>>>>>>>> It
>>>>>>>>>> does not work very well for large templates.
>>>>>>>>>> That code creates a writer which buffers up the whole thing, then
>>>>>>>>>> does a
>>>>>>>>>> toString() on it creating another copy. Then in some cases calls
>>>>>>>>>> substring() that creates yet another copy.
>>>>>>>>>>
>>>>>>>>>> The substring belonged to an old workaround, it has been removed.
>>>>>>>>>>
>>>>>>>>>> The StringWriter buffering is only done when interpolation is
>>>>>>>>>> needed
>>>>>>>>>>
>>>>>>>>> (that
>>>>>>>>> is, when the string literal is enclosed in double quotes and has
>>>>>>>>> some $
>>>>>>>>> or
>>>>>>>>> # inside). There could be some tricky ways of introducing some kind
>>>>>>>>> of
>>>>>>>>> lazy
>>>>>>>>> evaluation, that would resort to using the provided final writer
>>>>>>>>> when
>>>>>>>>> the
>>>>>>>>> value is meant to be rendered or to a buffered writer otherwise.
>>>>>>>>> But
>>>>>>>>> I'm
>>>>>>>>> not sure it is worth it, because it looks rather fragile and
>>>>>>>>> convoluted.
>>>>>>>>> Plus, you can generally avoid having big string literals. I don't
>>>>>>>>> know
>>>>>>>>> about your use case, but why can't something as:
>>>>>>>>>
>>>>>>>>> #set($foo = "#parse('big.vtl')") $foo
>>>>>>>>>
>>>>>>>>> be rewritten as:
>>>>>>>>>
>>>>>>>>> #parse('big.vtl')
>>>>>>>>>
>>>>>>>>> to avoid the buffering?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Claude
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ------------------------------------------------------------
>>>>>>>>> ---------
>>>>>>>>>
>>>>>>>>> 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
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
Re: 2.0 question
Posted by Claude Brisson <cl...@renegat.net>.
It's the VelocityView from the tools subproject, which uses a pool of
VelocityWriters. The engine itself doesn't know which writer is given as
argument.
Yes, it's *possible* to do it. But it also implies breaking backward
compatibility for the o.a.v.io.Filter API, all for the sake of
ASTStringLiteral's interpolation performance for big interpolated
templates (there is no other concerned case in standard use).
So the trade-off is not worth. Plus, even if we try to be as fast as we
can with the current code paradigms, the whole engine would have to be
rewritten using nio buffers and file mapping to address a real
performance gain. This is not the purpose of the 2.0 version.
Claude
On 13/12/2016 16:01, Alex Fedotov wrote:
> I guess it depends what Writer is used.
> StringBuilderWriter.append(CharSequence) is overloaded.
>
> I thought that Velocity is using a pool of VelocityWriter instances, not an
> OutputStreamWriter. If that is so you can overload append(CharSequence) as
> needed in VelocityWriter.
>
> JDK default implementation is very poor (creating potentially two copies of
> data in subSequence and toString).
>
> public Writer append(CharSequence csq, int start, int end) throws
> IOException {
> CharSequence cs = (csq == null ? "null" : csq);
> write(cs.subSequence(start, end).toString());
> return this;
> }
>
>
> On Tue, Dec 13, 2016 at 8:48 AM, Claude Brisson <cl...@renegat.net> wrote:
>
>> Reducing useless synchronizations by using a StringBuilderWriter was easy.
>>
>> But about using CharSequences instead of Strings, after a quick look, it
>> doesn't look so promising: did you know that Writer.append(CharSequence)
>> does call Writer.write(sequence.toString()), which will itself use
>> string.getChars(), at leats in its default version?!
>>
>> The most frightening about this code in java.io.Writer is that whenever
>> you pass big strings to it, it will each time allocate big buffers instead
>> of, for instance, copy small portions to output inside a loop. Time needed
>> for memory copy is often negligible, whereas time needed to allocate big
>> chunks of memory is rather annoying. Twice as annoying when done twice.
>>
>> There is no point in trying to avoid the toString() call in Velocity if
>> the i/o lower layer will call it anyway.
>>
>> Claude
>>
>>
>>
>> On 12/12/2016 18:15, Alex Fedotov wrote:
>>
>>> I don't know if something like this is in scope fro 2.0, but for more
>>> advanced manipulation of AST it would be helpful if creation of all AST
>>> nodes would be delegated to some kind of configurable factory class.
>>>
>>> This way if I wanted to replace ASTStringLiteral with a subclass of
>>> ASTStringLiteral for optimization purposes (say use a chunked buffer, or
>>> something similar) I could then setup my own factory and create an
>>> instance
>>> of a subclass.
>>>
>>> Alex
>>>
>>>
>>> On Mon, Dec 12, 2016 at 11:52 AM, Claude Brisson <cl...@renegat.net>
>>> wrote:
>>>
>>> Yes, I also thought of that. But a particular care must be taken in
>>>> upstream code that may expect to encounter String values and may
>>>> otherwise
>>>> call toString(), voiding the benefice. Worth looking at, though ; it must
>>>> not be too difficult to have this code take CharSequence values into
>>>> account.
>>>>
>>>> Claude
>>>>
>>>>
>>>>
>>>> On 12/12/2016 15:58, Alex Fedotov wrote:
>>>>
>>>> I am not sure if it's possible to assume that ASTStringLiteral.value()
>>>>> returns a CharSequence and not a String. If that was the case you could
>>>>> just return the StringBuilder directly (StringBuilder implements
>>>>> CharSequence) without calling StringBuilder.toString(). This would avoid
>>>>> making one more copy of the data.
>>>>>
>>>>> Same may apply in other places.
>>>>>
>>>>> Alex
>>>>>
>>>>>
>>>>> On Sun, Dec 11, 2016 at 12:28 PM, Claude Brisson <cl...@renegat.net>
>>>>> wrote:
>>>>>
>>>>> Thanks. I'll review those StringBuffer uses.
>>>>>
>>>>>> For the ASTStringLiteral, I see there's already a StringBuilderWriter
>>>>>> in
>>>>>> commons-io that I may borrow...
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/12/2016 17:07, Alex Fedotov wrote:
>>>>>>
>>>>>> Great thank you.
>>>>>>
>>>>>>> Actually I would recommend removing the StringWriter. StringWriter is
>>>>>>> using
>>>>>>> StringBuffer inside which is synchronized.
>>>>>>> In case of ASTStringLiteral there is no need to synchronize anything.
>>>>>>> There
>>>>>>> must be a Writer implementation based on StringBuilder instead.
>>>>>>>
>>>>>>> I have a run a search on the latest 2.0 code and it seems like there
>>>>>>> is
>>>>>>> a
>>>>>>> number of places where StringBuffer is used.
>>>>>>> Unless synchronization is required those usages should really be
>>>>>>> replaced
>>>>>>> with StringBuilder(s).
>>>>>>>
>>>>>>>
>>>>>>> ============================================================
>>>>>>> =================
>>>>>>>
>>>>>>> As far our usage of large string literal it was in a legacy framework
>>>>>>> that
>>>>>>> we had.
>>>>>>> It was creating a system of macros to render "tiles" on a page.
>>>>>>>
>>>>>>> Similar to this syntax (not real):
>>>>>>>
>>>>>>> @#PageSection(param1, param2....)
>>>>>>> @#SubSection("tileName1")
>>>>>>> some html
>>>>>>> #end
>>>>>>> @#SubSection("tileName2")
>>>>>>> some other html
>>>>>>> #end
>>>>>>> #SubSection("tileName3", "/blah/blah/section.vtl")
>>>>>>> #end
>>>>>>>
>>>>>>> Velocity macros with body allow only one body block (special parameter
>>>>>>> really).
>>>>>>> We needed multiple named body blocks which we could render in the
>>>>>>> correct
>>>>>>> spots inside of the macro.
>>>>>>>
>>>>>>> We are no longer using this framework, but that is where we had large
>>>>>>> string literals appearing.
>>>>>>> I think we replaced the StringBuffer with some unsynchronized chunked
>>>>>>> implementation to avoid allocation of very large strings and
>>>>>>> unnecessary
>>>>>>> synchronzation.
>>>>>>>
>>>>>>> Alex
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Sun, Dec 11, 2016 at 7:18 AM, Claude Brisson <cl...@renegat.net>
>>>>>>> wrote:
>>>>>>>
>>>>>>> FYI, we got rid of the Executor pattern in the events API, and we now
>>>>>>>
>>>>>>> always provide the current Context when calling handlers.
>>>>>>>>
>>>>>>>> On 29/11/2016 23:25, Alex Fedotov wrote:
>>>>>>>> [...]
>>>>>>>>
>>>>>>>> We have run into some other inefficient places. For example
>>>>>>>>
>>>>>>>> ASTStringLiteral is buffering the entire content in the StringWriter.
>>>>>>>>> It
>>>>>>>>> does not work very well for large templates.
>>>>>>>>> That code creates a writer which buffers up the whole thing, then
>>>>>>>>> does a
>>>>>>>>> toString() on it creating another copy. Then in some cases calls
>>>>>>>>> substring() that creates yet another copy.
>>>>>>>>>
>>>>>>>>> The substring belonged to an old workaround, it has been removed.
>>>>>>>>>
>>>>>>>>> The StringWriter buffering is only done when interpolation is needed
>>>>>>>> (that
>>>>>>>> is, when the string literal is enclosed in double quotes and has
>>>>>>>> some $
>>>>>>>> or
>>>>>>>> # inside). There could be some tricky ways of introducing some kind
>>>>>>>> of
>>>>>>>> lazy
>>>>>>>> evaluation, that would resort to using the provided final writer when
>>>>>>>> the
>>>>>>>> value is meant to be rendered or to a buffered writer otherwise. But
>>>>>>>> I'm
>>>>>>>> not sure it is worth it, because it looks rather fragile and
>>>>>>>> convoluted.
>>>>>>>> Plus, you can generally avoid having big string literals. I don't
>>>>>>>> know
>>>>>>>> about your use case, but why can't something as:
>>>>>>>>
>>>>>>>> #set($foo = "#parse('big.vtl')") $foo
>>>>>>>>
>>>>>>>> be rewritten as:
>>>>>>>>
>>>>>>>> #parse('big.vtl')
>>>>>>>>
>>>>>>>> to avoid the buffering?
>>>>>>>>
>>>>>>>>
>>>>>>>> Claude
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> ------------------------------------------------------------
>>>>>>>> ---------
>>>>>>>>
>>>>>>> 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
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: 2.0 question
Posted by Alex Fedotov <al...@kayak.com>.
I guess it depends what Writer is used.
StringBuilderWriter.append(CharSequence) is overloaded.
I thought that Velocity is using a pool of VelocityWriter instances, not an
OutputStreamWriter. If that is so you can overload append(CharSequence) as
needed in VelocityWriter.
JDK default implementation is very poor (creating potentially two copies of
data in subSequence and toString).
public Writer append(CharSequence csq, int start, int end) throws
IOException {
CharSequence cs = (csq == null ? "null" : csq);
write(cs.subSequence(start, end).toString());
return this;
}
On Tue, Dec 13, 2016 at 8:48 AM, Claude Brisson <cl...@renegat.net> wrote:
> Reducing useless synchronizations by using a StringBuilderWriter was easy.
>
> But about using CharSequences instead of Strings, after a quick look, it
> doesn't look so promising: did you know that Writer.append(CharSequence)
> does call Writer.write(sequence.toString()), which will itself use
> string.getChars(), at leats in its default version?!
>
> The most frightening about this code in java.io.Writer is that whenever
> you pass big strings to it, it will each time allocate big buffers instead
> of, for instance, copy small portions to output inside a loop. Time needed
> for memory copy is often negligible, whereas time needed to allocate big
> chunks of memory is rather annoying. Twice as annoying when done twice.
>
> There is no point in trying to avoid the toString() call in Velocity if
> the i/o lower layer will call it anyway.
>
> Claude
>
>
>
> On 12/12/2016 18:15, Alex Fedotov wrote:
>
>> I don't know if something like this is in scope fro 2.0, but for more
>> advanced manipulation of AST it would be helpful if creation of all AST
>> nodes would be delegated to some kind of configurable factory class.
>>
>> This way if I wanted to replace ASTStringLiteral with a subclass of
>> ASTStringLiteral for optimization purposes (say use a chunked buffer, or
>> something similar) I could then setup my own factory and create an
>> instance
>> of a subclass.
>>
>> Alex
>>
>>
>> On Mon, Dec 12, 2016 at 11:52 AM, Claude Brisson <cl...@renegat.net>
>> wrote:
>>
>> Yes, I also thought of that. But a particular care must be taken in
>>> upstream code that may expect to encounter String values and may
>>> otherwise
>>> call toString(), voiding the benefice. Worth looking at, though ; it must
>>> not be too difficult to have this code take CharSequence values into
>>> account.
>>>
>>> Claude
>>>
>>>
>>>
>>> On 12/12/2016 15:58, Alex Fedotov wrote:
>>>
>>> I am not sure if it's possible to assume that ASTStringLiteral.value()
>>>> returns a CharSequence and not a String. If that was the case you could
>>>> just return the StringBuilder directly (StringBuilder implements
>>>> CharSequence) without calling StringBuilder.toString(). This would avoid
>>>> making one more copy of the data.
>>>>
>>>> Same may apply in other places.
>>>>
>>>> Alex
>>>>
>>>>
>>>> On Sun, Dec 11, 2016 at 12:28 PM, Claude Brisson <cl...@renegat.net>
>>>> wrote:
>>>>
>>>> Thanks. I'll review those StringBuffer uses.
>>>>
>>>>> For the ASTStringLiteral, I see there's already a StringBuilderWriter
>>>>> in
>>>>> commons-io that I may borrow...
>>>>>
>>>>>
>>>>>
>>>>> On 11/12/2016 17:07, Alex Fedotov wrote:
>>>>>
>>>>> Great thank you.
>>>>>
>>>>>> Actually I would recommend removing the StringWriter. StringWriter is
>>>>>> using
>>>>>> StringBuffer inside which is synchronized.
>>>>>> In case of ASTStringLiteral there is no need to synchronize anything.
>>>>>> There
>>>>>> must be a Writer implementation based on StringBuilder instead.
>>>>>>
>>>>>> I have a run a search on the latest 2.0 code and it seems like there
>>>>>> is
>>>>>> a
>>>>>> number of places where StringBuffer is used.
>>>>>> Unless synchronization is required those usages should really be
>>>>>> replaced
>>>>>> with StringBuilder(s).
>>>>>>
>>>>>>
>>>>>> ============================================================
>>>>>> =================
>>>>>>
>>>>>> As far our usage of large string literal it was in a legacy framework
>>>>>> that
>>>>>> we had.
>>>>>> It was creating a system of macros to render "tiles" on a page.
>>>>>>
>>>>>> Similar to this syntax (not real):
>>>>>>
>>>>>> @#PageSection(param1, param2....)
>>>>>> @#SubSection("tileName1")
>>>>>> some html
>>>>>> #end
>>>>>> @#SubSection("tileName2")
>>>>>> some other html
>>>>>> #end
>>>>>> #SubSection("tileName3", "/blah/blah/section.vtl")
>>>>>> #end
>>>>>>
>>>>>> Velocity macros with body allow only one body block (special parameter
>>>>>> really).
>>>>>> We needed multiple named body blocks which we could render in the
>>>>>> correct
>>>>>> spots inside of the macro.
>>>>>>
>>>>>> We are no longer using this framework, but that is where we had large
>>>>>> string literals appearing.
>>>>>> I think we replaced the StringBuffer with some unsynchronized chunked
>>>>>> implementation to avoid allocation of very large strings and
>>>>>> unnecessary
>>>>>> synchronzation.
>>>>>>
>>>>>> Alex
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Sun, Dec 11, 2016 at 7:18 AM, Claude Brisson <cl...@renegat.net>
>>>>>> wrote:
>>>>>>
>>>>>> FYI, we got rid of the Executor pattern in the events API, and we now
>>>>>>
>>>>>> always provide the current Context when calling handlers.
>>>>>>>
>>>>>>>
>>>>>>> On 29/11/2016 23:25, Alex Fedotov wrote:
>>>>>>> [...]
>>>>>>>
>>>>>>> We have run into some other inefficient places. For example
>>>>>>>
>>>>>>> ASTStringLiteral is buffering the entire content in the StringWriter.
>>>>>>>> It
>>>>>>>> does not work very well for large templates.
>>>>>>>> That code creates a writer which buffers up the whole thing, then
>>>>>>>> does a
>>>>>>>> toString() on it creating another copy. Then in some cases calls
>>>>>>>> substring() that creates yet another copy.
>>>>>>>>
>>>>>>>> The substring belonged to an old workaround, it has been removed.
>>>>>>>>
>>>>>>>> The StringWriter buffering is only done when interpolation is needed
>>>>>>> (that
>>>>>>> is, when the string literal is enclosed in double quotes and has
>>>>>>> some $
>>>>>>> or
>>>>>>> # inside). There could be some tricky ways of introducing some kind
>>>>>>> of
>>>>>>> lazy
>>>>>>> evaluation, that would resort to using the provided final writer when
>>>>>>> the
>>>>>>> value is meant to be rendered or to a buffered writer otherwise. But
>>>>>>> I'm
>>>>>>> not sure it is worth it, because it looks rather fragile and
>>>>>>> convoluted.
>>>>>>> Plus, you can generally avoid having big string literals. I don't
>>>>>>> know
>>>>>>> about your use case, but why can't something as:
>>>>>>>
>>>>>>> #set($foo = "#parse('big.vtl')") $foo
>>>>>>>
>>>>>>> be rewritten as:
>>>>>>>
>>>>>>> #parse('big.vtl')
>>>>>>>
>>>>>>> to avoid the buffering?
>>>>>>>
>>>>>>>
>>>>>>> Claude
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ------------------------------------------------------------
>>>>>>> ---------
>>>>>>>
>>>>>> 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: 2.0 question
Posted by Claude Brisson <cl...@renegat.net>.
Reducing useless synchronizations by using a StringBuilderWriter was easy.
But about using CharSequences instead of Strings, after a quick look, it
doesn't look so promising: did you know that Writer.append(CharSequence)
does call Writer.write(sequence.toString()), which will itself use
string.getChars(), at leats in its default version?!
The most frightening about this code in java.io.Writer is that whenever
you pass big strings to it, it will each time allocate big buffers
instead of, for instance, copy small portions to output inside a loop.
Time needed for memory copy is often negligible, whereas time needed to
allocate big chunks of memory is rather annoying. Twice as annoying when
done twice.
There is no point in trying to avoid the toString() call in Velocity if
the i/o lower layer will call it anyway.
Claude
On 12/12/2016 18:15, Alex Fedotov wrote:
> I don't know if something like this is in scope fro 2.0, but for more
> advanced manipulation of AST it would be helpful if creation of all AST
> nodes would be delegated to some kind of configurable factory class.
>
> This way if I wanted to replace ASTStringLiteral with a subclass of
> ASTStringLiteral for optimization purposes (say use a chunked buffer, or
> something similar) I could then setup my own factory and create an instance
> of a subclass.
>
> Alex
>
>
> On Mon, Dec 12, 2016 at 11:52 AM, Claude Brisson <cl...@renegat.net> wrote:
>
>> Yes, I also thought of that. But a particular care must be taken in
>> upstream code that may expect to encounter String values and may otherwise
>> call toString(), voiding the benefice. Worth looking at, though ; it must
>> not be too difficult to have this code take CharSequence values into
>> account.
>>
>> Claude
>>
>>
>>
>> On 12/12/2016 15:58, Alex Fedotov wrote:
>>
>>> I am not sure if it's possible to assume that ASTStringLiteral.value()
>>> returns a CharSequence and not a String. If that was the case you could
>>> just return the StringBuilder directly (StringBuilder implements
>>> CharSequence) without calling StringBuilder.toString(). This would avoid
>>> making one more copy of the data.
>>>
>>> Same may apply in other places.
>>>
>>> Alex
>>>
>>>
>>> On Sun, Dec 11, 2016 at 12:28 PM, Claude Brisson <cl...@renegat.net>
>>> wrote:
>>>
>>> Thanks. I'll review those StringBuffer uses.
>>>> For the ASTStringLiteral, I see there's already a StringBuilderWriter in
>>>> commons-io that I may borrow...
>>>>
>>>>
>>>>
>>>> On 11/12/2016 17:07, Alex Fedotov wrote:
>>>>
>>>> Great thank you.
>>>>> Actually I would recommend removing the StringWriter. StringWriter is
>>>>> using
>>>>> StringBuffer inside which is synchronized.
>>>>> In case of ASTStringLiteral there is no need to synchronize anything.
>>>>> There
>>>>> must be a Writer implementation based on StringBuilder instead.
>>>>>
>>>>> I have a run a search on the latest 2.0 code and it seems like there is
>>>>> a
>>>>> number of places where StringBuffer is used.
>>>>> Unless synchronization is required those usages should really be
>>>>> replaced
>>>>> with StringBuilder(s).
>>>>>
>>>>>
>>>>> ============================================================
>>>>> =================
>>>>>
>>>>> As far our usage of large string literal it was in a legacy framework
>>>>> that
>>>>> we had.
>>>>> It was creating a system of macros to render "tiles" on a page.
>>>>>
>>>>> Similar to this syntax (not real):
>>>>>
>>>>> @#PageSection(param1, param2....)
>>>>> @#SubSection("tileName1")
>>>>> some html
>>>>> #end
>>>>> @#SubSection("tileName2")
>>>>> some other html
>>>>> #end
>>>>> #SubSection("tileName3", "/blah/blah/section.vtl")
>>>>> #end
>>>>>
>>>>> Velocity macros with body allow only one body block (special parameter
>>>>> really).
>>>>> We needed multiple named body blocks which we could render in the
>>>>> correct
>>>>> spots inside of the macro.
>>>>>
>>>>> We are no longer using this framework, but that is where we had large
>>>>> string literals appearing.
>>>>> I think we replaced the StringBuffer with some unsynchronized chunked
>>>>> implementation to avoid allocation of very large strings and unnecessary
>>>>> synchronzation.
>>>>>
>>>>> Alex
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Sun, Dec 11, 2016 at 7:18 AM, Claude Brisson <cl...@renegat.net>
>>>>> wrote:
>>>>>
>>>>> FYI, we got rid of the Executor pattern in the events API, and we now
>>>>>
>>>>>> always provide the current Context when calling handlers.
>>>>>>
>>>>>>
>>>>>> On 29/11/2016 23:25, Alex Fedotov wrote:
>>>>>> [...]
>>>>>>
>>>>>> We have run into some other inefficient places. For example
>>>>>>
>>>>>>> ASTStringLiteral is buffering the entire content in the StringWriter.
>>>>>>> It
>>>>>>> does not work very well for large templates.
>>>>>>> That code creates a writer which buffers up the whole thing, then
>>>>>>> does a
>>>>>>> toString() on it creating another copy. Then in some cases calls
>>>>>>> substring() that creates yet another copy.
>>>>>>>
>>>>>>> The substring belonged to an old workaround, it has been removed.
>>>>>>>
>>>>>> The StringWriter buffering is only done when interpolation is needed
>>>>>> (that
>>>>>> is, when the string literal is enclosed in double quotes and has some $
>>>>>> or
>>>>>> # inside). There could be some tricky ways of introducing some kind of
>>>>>> lazy
>>>>>> evaluation, that would resort to using the provided final writer when
>>>>>> the
>>>>>> value is meant to be rendered or to a buffered writer otherwise. But
>>>>>> I'm
>>>>>> not sure it is worth it, because it looks rather fragile and
>>>>>> convoluted.
>>>>>> Plus, you can generally avoid having big string literals. I don't know
>>>>>> about your use case, but why can't something as:
>>>>>>
>>>>>> #set($foo = "#parse('big.vtl')") $foo
>>>>>>
>>>>>> be rewritten as:
>>>>>>
>>>>>> #parse('big.vtl')
>>>>>>
>>>>>> to avoid the buffering?
>>>>>>
>>>>>>
>>>>>> Claude
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>> 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: 2.0 question
Posted by Alex Fedotov <al...@kayak.com>.
I don't know if something like this is in scope fro 2.0, but for more
advanced manipulation of AST it would be helpful if creation of all AST
nodes would be delegated to some kind of configurable factory class.
This way if I wanted to replace ASTStringLiteral with a subclass of
ASTStringLiteral for optimization purposes (say use a chunked buffer, or
something similar) I could then setup my own factory and create an instance
of a subclass.
Alex
On Mon, Dec 12, 2016 at 11:52 AM, Claude Brisson <cl...@renegat.net> wrote:
> Yes, I also thought of that. But a particular care must be taken in
> upstream code that may expect to encounter String values and may otherwise
> call toString(), voiding the benefice. Worth looking at, though ; it must
> not be too difficult to have this code take CharSequence values into
> account.
>
> Claude
>
>
>
> On 12/12/2016 15:58, Alex Fedotov wrote:
>
>> I am not sure if it's possible to assume that ASTStringLiteral.value()
>> returns a CharSequence and not a String. If that was the case you could
>> just return the StringBuilder directly (StringBuilder implements
>> CharSequence) without calling StringBuilder.toString(). This would avoid
>> making one more copy of the data.
>>
>> Same may apply in other places.
>>
>> Alex
>>
>>
>> On Sun, Dec 11, 2016 at 12:28 PM, Claude Brisson <cl...@renegat.net>
>> wrote:
>>
>> Thanks. I'll review those StringBuffer uses.
>>>
>>> For the ASTStringLiteral, I see there's already a StringBuilderWriter in
>>> commons-io that I may borrow...
>>>
>>>
>>>
>>> On 11/12/2016 17:07, Alex Fedotov wrote:
>>>
>>> Great thank you.
>>>>
>>>> Actually I would recommend removing the StringWriter. StringWriter is
>>>> using
>>>> StringBuffer inside which is synchronized.
>>>> In case of ASTStringLiteral there is no need to synchronize anything.
>>>> There
>>>> must be a Writer implementation based on StringBuilder instead.
>>>>
>>>> I have a run a search on the latest 2.0 code and it seems like there is
>>>> a
>>>> number of places where StringBuffer is used.
>>>> Unless synchronization is required those usages should really be
>>>> replaced
>>>> with StringBuilder(s).
>>>>
>>>>
>>>> ============================================================
>>>> =================
>>>>
>>>> As far our usage of large string literal it was in a legacy framework
>>>> that
>>>> we had.
>>>> It was creating a system of macros to render "tiles" on a page.
>>>>
>>>> Similar to this syntax (not real):
>>>>
>>>> @#PageSection(param1, param2....)
>>>> @#SubSection("tileName1")
>>>> some html
>>>> #end
>>>> @#SubSection("tileName2")
>>>> some other html
>>>> #end
>>>> #SubSection("tileName3", "/blah/blah/section.vtl")
>>>> #end
>>>>
>>>> Velocity macros with body allow only one body block (special parameter
>>>> really).
>>>> We needed multiple named body blocks which we could render in the
>>>> correct
>>>> spots inside of the macro.
>>>>
>>>> We are no longer using this framework, but that is where we had large
>>>> string literals appearing.
>>>> I think we replaced the StringBuffer with some unsynchronized chunked
>>>> implementation to avoid allocation of very large strings and unnecessary
>>>> synchronzation.
>>>>
>>>> Alex
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Sun, Dec 11, 2016 at 7:18 AM, Claude Brisson <cl...@renegat.net>
>>>> wrote:
>>>>
>>>> FYI, we got rid of the Executor pattern in the events API, and we now
>>>>
>>>>> always provide the current Context when calling handlers.
>>>>>
>>>>>
>>>>> On 29/11/2016 23:25, Alex Fedotov wrote:
>>>>> [...]
>>>>>
>>>>> We have run into some other inefficient places. For example
>>>>>
>>>>>> ASTStringLiteral is buffering the entire content in the StringWriter.
>>>>>> It
>>>>>> does not work very well for large templates.
>>>>>> That code creates a writer which buffers up the whole thing, then
>>>>>> does a
>>>>>> toString() on it creating another copy. Then in some cases calls
>>>>>> substring() that creates yet another copy.
>>>>>>
>>>>>> The substring belonged to an old workaround, it has been removed.
>>>>>>
>>>>> The StringWriter buffering is only done when interpolation is needed
>>>>> (that
>>>>> is, when the string literal is enclosed in double quotes and has some $
>>>>> or
>>>>> # inside). There could be some tricky ways of introducing some kind of
>>>>> lazy
>>>>> evaluation, that would resort to using the provided final writer when
>>>>> the
>>>>> value is meant to be rendered or to a buffered writer otherwise. But
>>>>> I'm
>>>>> not sure it is worth it, because it looks rather fragile and
>>>>> convoluted.
>>>>> Plus, you can generally avoid having big string literals. I don't know
>>>>> about your use case, but why can't something as:
>>>>>
>>>>> #set($foo = "#parse('big.vtl')") $foo
>>>>>
>>>>> be rewritten as:
>>>>>
>>>>> #parse('big.vtl')
>>>>>
>>>>> to avoid the buffering?
>>>>>
>>>>>
>>>>> Claude
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>> 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: 2.0 question
Posted by Claude Brisson <cl...@renegat.net>.
Yes, I also thought of that. But a particular care must be taken in
upstream code that may expect to encounter String values and may
otherwise call toString(), voiding the benefice. Worth looking at,
though ; it must not be too difficult to have this code take
CharSequence values into account.
Claude
On 12/12/2016 15:58, Alex Fedotov wrote:
> I am not sure if it's possible to assume that ASTStringLiteral.value()
> returns a CharSequence and not a String. If that was the case you could
> just return the StringBuilder directly (StringBuilder implements
> CharSequence) without calling StringBuilder.toString(). This would avoid
> making one more copy of the data.
>
> Same may apply in other places.
>
> Alex
>
>
> On Sun, Dec 11, 2016 at 12:28 PM, Claude Brisson <cl...@renegat.net> wrote:
>
>> Thanks. I'll review those StringBuffer uses.
>>
>> For the ASTStringLiteral, I see there's already a StringBuilderWriter in
>> commons-io that I may borrow...
>>
>>
>>
>> On 11/12/2016 17:07, Alex Fedotov wrote:
>>
>>> Great thank you.
>>>
>>> Actually I would recommend removing the StringWriter. StringWriter is
>>> using
>>> StringBuffer inside which is synchronized.
>>> In case of ASTStringLiteral there is no need to synchronize anything.
>>> There
>>> must be a Writer implementation based on StringBuilder instead.
>>>
>>> I have a run a search on the latest 2.0 code and it seems like there is a
>>> number of places where StringBuffer is used.
>>> Unless synchronization is required those usages should really be replaced
>>> with StringBuilder(s).
>>>
>>>
>>> ============================================================
>>> =================
>>>
>>> As far our usage of large string literal it was in a legacy framework that
>>> we had.
>>> It was creating a system of macros to render "tiles" on a page.
>>>
>>> Similar to this syntax (not real):
>>>
>>> @#PageSection(param1, param2....)
>>> @#SubSection("tileName1")
>>> some html
>>> #end
>>> @#SubSection("tileName2")
>>> some other html
>>> #end
>>> #SubSection("tileName3", "/blah/blah/section.vtl")
>>> #end
>>>
>>> Velocity macros with body allow only one body block (special parameter
>>> really).
>>> We needed multiple named body blocks which we could render in the correct
>>> spots inside of the macro.
>>>
>>> We are no longer using this framework, but that is where we had large
>>> string literals appearing.
>>> I think we replaced the StringBuffer with some unsynchronized chunked
>>> implementation to avoid allocation of very large strings and unnecessary
>>> synchronzation.
>>>
>>> Alex
>>>
>>>
>>>
>>>
>>>
>>> On Sun, Dec 11, 2016 at 7:18 AM, Claude Brisson <cl...@renegat.net>
>>> wrote:
>>>
>>> FYI, we got rid of the Executor pattern in the events API, and we now
>>>> always provide the current Context when calling handlers.
>>>>
>>>>
>>>> On 29/11/2016 23:25, Alex Fedotov wrote:
>>>> [...]
>>>>
>>>> We have run into some other inefficient places. For example
>>>>> ASTStringLiteral is buffering the entire content in the StringWriter. It
>>>>> does not work very well for large templates.
>>>>> That code creates a writer which buffers up the whole thing, then does a
>>>>> toString() on it creating another copy. Then in some cases calls
>>>>> substring() that creates yet another copy.
>>>>>
>>>>> The substring belonged to an old workaround, it has been removed.
>>>> The StringWriter buffering is only done when interpolation is needed
>>>> (that
>>>> is, when the string literal is enclosed in double quotes and has some $
>>>> or
>>>> # inside). There could be some tricky ways of introducing some kind of
>>>> lazy
>>>> evaluation, that would resort to using the provided final writer when the
>>>> value is meant to be rendered or to a buffered writer otherwise. But I'm
>>>> not sure it is worth it, because it looks rather fragile and convoluted.
>>>> Plus, you can generally avoid having big string literals. I don't know
>>>> about your use case, but why can't something as:
>>>>
>>>> #set($foo = "#parse('big.vtl')") $foo
>>>>
>>>> be rewritten as:
>>>>
>>>> #parse('big.vtl')
>>>>
>>>> to avoid the buffering?
>>>>
>>>>
>>>> Claude
>>>>
>>>>
>>>>
>>>>
>> ---------------------------------------------------------------------
>> 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: 2.0 question
Posted by Alex Fedotov <al...@kayak.com>.
I am not sure if it's possible to assume that ASTStringLiteral.value()
returns a CharSequence and not a String. If that was the case you could
just return the StringBuilder directly (StringBuilder implements
CharSequence) without calling StringBuilder.toString(). This would avoid
making one more copy of the data.
Same may apply in other places.
Alex
On Sun, Dec 11, 2016 at 12:28 PM, Claude Brisson <cl...@renegat.net> wrote:
> Thanks. I'll review those StringBuffer uses.
>
> For the ASTStringLiteral, I see there's already a StringBuilderWriter in
> commons-io that I may borrow...
>
>
>
> On 11/12/2016 17:07, Alex Fedotov wrote:
>
>> Great thank you.
>>
>> Actually I would recommend removing the StringWriter. StringWriter is
>> using
>> StringBuffer inside which is synchronized.
>> In case of ASTStringLiteral there is no need to synchronize anything.
>> There
>> must be a Writer implementation based on StringBuilder instead.
>>
>> I have a run a search on the latest 2.0 code and it seems like there is a
>> number of places where StringBuffer is used.
>> Unless synchronization is required those usages should really be replaced
>> with StringBuilder(s).
>>
>>
>> ============================================================
>> =================
>>
>> As far our usage of large string literal it was in a legacy framework that
>> we had.
>> It was creating a system of macros to render "tiles" on a page.
>>
>> Similar to this syntax (not real):
>>
>> @#PageSection(param1, param2....)
>> @#SubSection("tileName1")
>> some html
>> #end
>> @#SubSection("tileName2")
>> some other html
>> #end
>> #SubSection("tileName3", "/blah/blah/section.vtl")
>> #end
>>
>> Velocity macros with body allow only one body block (special parameter
>> really).
>> We needed multiple named body blocks which we could render in the correct
>> spots inside of the macro.
>>
>> We are no longer using this framework, but that is where we had large
>> string literals appearing.
>> I think we replaced the StringBuffer with some unsynchronized chunked
>> implementation to avoid allocation of very large strings and unnecessary
>> synchronzation.
>>
>> Alex
>>
>>
>>
>>
>>
>> On Sun, Dec 11, 2016 at 7:18 AM, Claude Brisson <cl...@renegat.net>
>> wrote:
>>
>> FYI, we got rid of the Executor pattern in the events API, and we now
>>> always provide the current Context when calling handlers.
>>>
>>>
>>> On 29/11/2016 23:25, Alex Fedotov wrote:
>>> [...]
>>>
>>> We have run into some other inefficient places. For example
>>>> ASTStringLiteral is buffering the entire content in the StringWriter. It
>>>> does not work very well for large templates.
>>>> That code creates a writer which buffers up the whole thing, then does a
>>>> toString() on it creating another copy. Then in some cases calls
>>>> substring() that creates yet another copy.
>>>>
>>>> The substring belonged to an old workaround, it has been removed.
>>>
>>> The StringWriter buffering is only done when interpolation is needed
>>> (that
>>> is, when the string literal is enclosed in double quotes and has some $
>>> or
>>> # inside). There could be some tricky ways of introducing some kind of
>>> lazy
>>> evaluation, that would resort to using the provided final writer when the
>>> value is meant to be rendered or to a buffered writer otherwise. But I'm
>>> not sure it is worth it, because it looks rather fragile and convoluted.
>>> Plus, you can generally avoid having big string literals. I don't know
>>> about your use case, but why can't something as:
>>>
>>> #set($foo = "#parse('big.vtl')") $foo
>>>
>>> be rewritten as:
>>>
>>> #parse('big.vtl')
>>>
>>> to avoid the buffering?
>>>
>>>
>>> Claude
>>>
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
Re: 2.0 question
Posted by Claude Brisson <cl...@renegat.net>.
Thanks. I'll review those StringBuffer uses.
For the ASTStringLiteral, I see there's already a StringBuilderWriter in
commons-io that I may borrow...
On 11/12/2016 17:07, Alex Fedotov wrote:
> Great thank you.
>
> Actually I would recommend removing the StringWriter. StringWriter is using
> StringBuffer inside which is synchronized.
> In case of ASTStringLiteral there is no need to synchronize anything. There
> must be a Writer implementation based on StringBuilder instead.
>
> I have a run a search on the latest 2.0 code and it seems like there is a
> number of places where StringBuffer is used.
> Unless synchronization is required those usages should really be replaced
> with StringBuilder(s).
>
>
> =============================================================================
>
> As far our usage of large string literal it was in a legacy framework that
> we had.
> It was creating a system of macros to render "tiles" on a page.
>
> Similar to this syntax (not real):
>
> @#PageSection(param1, param2....)
> @#SubSection("tileName1")
> some html
> #end
> @#SubSection("tileName2")
> some other html
> #end
> #SubSection("tileName3", "/blah/blah/section.vtl")
> #end
>
> Velocity macros with body allow only one body block (special parameter
> really).
> We needed multiple named body blocks which we could render in the correct
> spots inside of the macro.
>
> We are no longer using this framework, but that is where we had large
> string literals appearing.
> I think we replaced the StringBuffer with some unsynchronized chunked
> implementation to avoid allocation of very large strings and unnecessary
> synchronzation.
>
> Alex
>
>
>
>
>
> On Sun, Dec 11, 2016 at 7:18 AM, Claude Brisson <cl...@renegat.net> wrote:
>
>> FYI, we got rid of the Executor pattern in the events API, and we now
>> always provide the current Context when calling handlers.
>>
>>
>> On 29/11/2016 23:25, Alex Fedotov wrote:
>> [...]
>>
>>> We have run into some other inefficient places. For example
>>> ASTStringLiteral is buffering the entire content in the StringWriter. It
>>> does not work very well for large templates.
>>> That code creates a writer which buffers up the whole thing, then does a
>>> toString() on it creating another copy. Then in some cases calls
>>> substring() that creates yet another copy.
>>>
>> The substring belonged to an old workaround, it has been removed.
>>
>> The StringWriter buffering is only done when interpolation is needed (that
>> is, when the string literal is enclosed in double quotes and has some $ or
>> # inside). There could be some tricky ways of introducing some kind of lazy
>> evaluation, that would resort to using the provided final writer when the
>> value is meant to be rendered or to a buffered writer otherwise. But I'm
>> not sure it is worth it, because it looks rather fragile and convoluted.
>> Plus, you can generally avoid having big string literals. I don't know
>> about your use case, but why can't something as:
>>
>> #set($foo = "#parse('big.vtl')") $foo
>>
>> be rewritten as:
>>
>> #parse('big.vtl')
>>
>> to avoid the buffering?
>>
>>
>> Claude
>>
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: 2.0 question
Posted by Alex Fedotov <al...@kayak.com>.
Great thank you.
Actually I would recommend removing the StringWriter. StringWriter is using
StringBuffer inside which is synchronized.
In case of ASTStringLiteral there is no need to synchronize anything. There
must be a Writer implementation based on StringBuilder instead.
I have a run a search on the latest 2.0 code and it seems like there is a
number of places where StringBuffer is used.
Unless synchronization is required those usages should really be replaced
with StringBuilder(s).
=============================================================================
As far our usage of large string literal it was in a legacy framework that
we had.
It was creating a system of macros to render "tiles" on a page.
Similar to this syntax (not real):
@#PageSection(param1, param2....)
@#SubSection("tileName1")
some html
#end
@#SubSection("tileName2")
some other html
#end
#SubSection("tileName3", "/blah/blah/section.vtl")
#end
Velocity macros with body allow only one body block (special parameter
really).
We needed multiple named body blocks which we could render in the correct
spots inside of the macro.
We are no longer using this framework, but that is where we had large
string literals appearing.
I think we replaced the StringBuffer with some unsynchronized chunked
implementation to avoid allocation of very large strings and unnecessary
synchronzation.
Alex
On Sun, Dec 11, 2016 at 7:18 AM, Claude Brisson <cl...@renegat.net> wrote:
> FYI, we got rid of the Executor pattern in the events API, and we now
> always provide the current Context when calling handlers.
>
>
> On 29/11/2016 23:25, Alex Fedotov wrote:
> [...]
>
>> We have run into some other inefficient places. For example
>> ASTStringLiteral is buffering the entire content in the StringWriter. It
>> does not work very well for large templates.
>> That code creates a writer which buffers up the whole thing, then does a
>> toString() on it creating another copy. Then in some cases calls
>> substring() that creates yet another copy.
>>
>
> The substring belonged to an old workaround, it has been removed.
>
> The StringWriter buffering is only done when interpolation is needed (that
> is, when the string literal is enclosed in double quotes and has some $ or
> # inside). There could be some tricky ways of introducing some kind of lazy
> evaluation, that would resort to using the provided final writer when the
> value is meant to be rendered or to a buffered writer otherwise. But I'm
> not sure it is worth it, because it looks rather fragile and convoluted.
> Plus, you can generally avoid having big string literals. I don't know
> about your use case, but why can't something as:
>
> #set($foo = "#parse('big.vtl')") $foo
>
> be rewritten as:
>
> #parse('big.vtl')
>
> to avoid the buffering?
>
>
> Claude
>
>
>
Re: 2.0 question
Posted by Claude Brisson <cl...@renegat.net>.
FYI, we got rid of the Executor pattern in the events API, and we now
always provide the current Context when calling handlers.
On 29/11/2016 23:25, Alex Fedotov wrote:
[...]
> We have run into some other inefficient places. For example
> ASTStringLiteral is buffering the entire content in the StringWriter. It
> does not work very well for large templates.
> That code creates a writer which buffers up the whole thing, then does a
> toString() on it creating another copy. Then in some cases calls
> substring() that creates yet another copy.
The substring belonged to an old workaround, it has been removed.
The StringWriter buffering is only done when interpolation is needed
(that is, when the string literal is enclosed in double quotes and has
some $ or # inside). There could be some tricky ways of introducing some
kind of lazy evaluation, that would resort to using the provided final
writer when the value is meant to be rendered or to a buffered writer
otherwise. But I'm not sure it is worth it, because it looks rather
fragile and convoluted. Plus, you can generally avoid having big string
literals. I don't know about your use case, but why can't something as:
#set($foo = "#parse('big.vtl')") $foo
be rewritten as:
#parse('big.vtl')
to avoid the buffering?
Claude
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: 2.0 question
Posted by Alex Fedotov <al...@kayak.com>.
We use this as a QA tool to detect references that may not have been
properly escaped or otherwise marked as safe.
It does not run in a production environment, but does run in Selenium
environment which is also under load. Given the volume of requests we
always try to avoid allocating unnecessary objects.
Unfortunately the Context is the only place where we can get location
information for violation messages (template stack, macro stack, etc.)
IMO it would be much simple to just pass the Context to all event handlers
as one more standard parameter. People who don't Context parameter will not
use it.
It would solve the threading issue and avoid having "Handler instanceof
ContextAware" checks at the same time.
It seems like there is a some unnecessary work being done initializing
event cartridges, etc. and all of that just for the purpose of setting
RuntimeServices and Context references on them.
As far as creation of executors:
I understand that it was convenient to create the method
"iterateOverEventHandlers" and apply it for event handlers with different
parameters, but the cost of that is creation of the new executor instance
for every invocation.
It would be more efficient to just have a utility that returns a combined
list of handlers (if needed) instead of creating one iterator for each list
(application and context). Then the callback invocation code could just
walk the list and call the handlers.
We have run into some other inefficient places. For example
ASTStringLiteral is buffering the entire content in the StringWriter. It
does not work very well for large templates.
That code creates a writer which buffers up the whole thing, then does a
toString() on it creating another copy. Then in some cases calls
substring() that creates yet another copy.
I can dig up a few other things we fixed in our version if you guys are
interested.
Alex
On Mon, Nov 28, 2016 at 11:48 PM, Claude Brisson <cl...@renegat.net> wrote:
> Hi Alex.
>
> Thanks for your feedback.
>
> On 28/11/2016 20:52, Alex Fedotov wrote:
>
> Hello All,
>
> ContextAware interface in Velocity is not really workable in a
> multi-threaded environment.
> The handler is a singleton and setting the Context on the singleton from
> multiple threads is not going to work well, short of using some form of
> thread local.
>
>
> You are right about that. The ContextAware javadoc says:
>
> Important Note: Only local event handlers attached to the context (as
> opposed to global event handlers initialized in the velocity.properties
> file) should implement ContextAware. Since global event handlers are
> singletons individual requests will not be able to count on the correct
> context being loaded before a request.
>
> I agree that the site documentation could be clearer about it.
>
> Also the event handlers (such as insertion handler) are creating new
> instance of executor for each inserted value which is inefficient.
>
> I'm not sure that using a reference insertion event handler in the first
> place can be made efficient, and not sure either that the
> allocation/deallocation accounts for a lot of performance loss since modern
> JVMs do a really good job about allocation of small Java objects.
>
> The reference insertion event handler was initially meant to be a
> debugging and checking tool rather than a production tool. This being said,
> I known there can be some legitimate use cases to do it in production, but
> can I ask what is your specific use case, out of simple curiosity?
>
>
> Are you guys fixing any of this for 2.0?
>
>
> It is not planned to fix the issue for 2.0, but you can easily implement a
> custom implementation that will use a thread local member.
>
> The event management may be reviewed in a future version, but if you think
> this particular point should specifically be addressed, I strongly advise
> you to open an issue about it.
>
>
> Claude
>
>
Re: 2.0 question
Posted by Claude Brisson <cl...@renegat.net>.
Hi Alex.
Thanks for your feedback.
On 28/11/2016 20:52, Alex Fedotov wrote:
> Hello All,
>
> ContextAware interface in Velocity is not really workable in a
> multi-threaded environment.
> The handler is a singleton and setting the Context on the singleton from
> multiple threads is not going to work well, short of using some form of
> thread local.
You are right about that. The ContextAware javadoc says:
Important Note: Only local event handlers attached to the context
(as opposed to global event handlers initialized in the
velocity.properties file) should implement ContextAware. Since
global event handlers are singletons individual requests will not be
able to count on the correct context being loaded before a request.
I agree that the site documentation could be clearer about it.
> Also the event handlers (such as insertion handler) are creating new
> instance of executor for each inserted value which is inefficient.
I'm not sure that using a reference insertion event handler in the first
place can be made efficient, and not sure either that the
allocation/deallocation accounts for a lot of performance loss since
modern JVMs do a really good job about allocation of small Java objects.
The reference insertion event handler was initially meant to be a
debugging and checking tool rather than a production tool. This being
said, I known there can be some legitimate use cases to do it in
production, but can I ask what is your specific use case, out of simple
curiosity?
>
> Are you guys fixing any of this for 2.0?
It is not planned to fix the issue for 2.0, but you can easily implement
a custom implementation that will use a thread local member.
The event management may be reviewed in a future version, but if you
think this particular point should specifically be addressed, I strongly
advise you to open an issue about it.
Claude