Velocity 2.0 (trunk) macro synchronization
Hello Guys, We use Velocity quite a bit and are considering moving forward and deploying the trunk build in production even though there never was a 2.0 release. We currently use a custom 1.7 build with a couple of bugs fixed by us. One area where we are going to make some changes before it can go in production is the synchronization in the VelocimacroFactory and VelicimacroManager classes. It appears to be a mix of ConcurrentHashMaps and global synchronized blocks. Our code is highly threaded, loads quite a bit of templates dynamically at run-time and having these global locks would likely present a synchronization hotspot. Our plan is to see if we can cleanup the synchronization in this area keep it robust, but remove the global synchronized blocks. If you guys have any interest in accepting our contribution back please let me know what the process should be. Thank you, Alex
Re: Velocity 2.0 (trunk) macro synchronization
One more question. One of the other areas we making changes is around how
macros are called. Right now the lookup process is fairly rigid and can be
inefficient as well. Consider a couple of examples:
1. Scenario one - overload macro in the template This works now because the
lookup checks the namespace where the call is made first.
macros.vtl
#macro(B)
#end
#macro(A)
#B()
#end
template.vtl
=
#parse('macros.vtl')
#macro(A)
## overloaded macro A
#end
#A() ## this calls local macro A which is expected.
=
2. Scenario two - overload macro B - this does not work now.
template.vtl
=
#parse('macros.vtl')
#macro(B)
## overloaded macro B
#end
#A() ## this calls macro A which is then does NOT call overloaded B, since
the look-up would prefer the B from the same module where A is.
=
We have created a custom directive #call which is similar to the
RuntimeMacro, but allows more control over the lookup process. This also
allows stuff like:
#call('self::macroName')
#call('nameSpace::macroName')
The way every #parse(d) template is appended to the list of the
"macroLibraries" can be very bad for macro look-up speed:
#foreach( $item in $items )
#parse('item.vtl')
#end
You end-up with a lot of "item.vtl" references added to the list which then
cause a lot of duplicate macro lookups in namespaces.
We have fixes that avoid adding duplicate sequences of template names to
the macroLibraries list.
Do you guys have any plans to address this area?
We are also inclined to always use namespaces for macros, always allow
local macros to replace globals and keep the macros as ConcurrentHashMap in
the Template object.
This way VelocimacroManager can be dropped all together, there is no issue
with reloading templates while maintaining the VelocimacroManager in sync,
less synchronization problems, etc.
The flag "isFromLib" for macros would also not be needed, so all the hair
around maintaining it during the "init" phase could be removed as well.
Alex
On Fri, Mar 6, 2015 at 2:48 AM, Claude Brisson wrote:
> Hi.
>
> We'll publish the 2.0 one day, it's slowly moving...
>
> Thank you for your proposal. Of course, it is interesting for the project
> if you can share your 1.7 bugfixes as long as your synchronization work on
> the 2.0 branch.
>
> The process is rather easy: patches are to be submitted on JIRA :
> https://issues.apache.org/jira/browse/VELOCITY/
>
> Very small patches (too small to be considered intellectual property)
> don't need anything else. For more consequent patches, some people say you
> should at least add a sentence like "feel free to use this patch under the
> Apache License" in the issue.
>
>
> Claude
>
>
>
> On 04/03/2015 17:48, Alex Fedotov wrote:
>
>> Hello Guys,
>>
>> We use Velocity quite a bit and are considering moving forward and
>> deploying the trunk build in production even though there never was a 2.0
>> release. We currently use a custom 1.7 build with a couple of bugs fixed
>> by
>> us.
>>
>> One area where we are going to make some changes before it can go in
>> production is the synchronization in the VelocimacroFactory and
>> VelicimacroManager classes. It appears to be a mix of ConcurrentHashMaps
>> and global synchronized blocks. Our code is highly threaded, loads quite a
>> bit of templates dynamically at run-time and having these global locks
>> would likely present a synchronization hotspot.
>>
>> Our plan is to see if we can cleanup the synchronization in this area keep
>> it robust, but remove the global synchronized blocks.
>>
>> If you guys have any interest in accepting our contribution back please
>> let
>> me know what the process should be.
>>
>> Thank you,
>> Alex
>>
>>
>
Re: Velocity 2.0 (trunk) macro synchronization
Sure it can be done.
I would still keep the macros in templates, including the global templates
(i.e. library files). I would maintain the list of global Template objects.
Depending on the settings, during the macro lookup iterate over the global
Template objects first, if not found there do the normal lookup in the
calling template or in the Template list (macroLibraries).
Alex
On Fri, Mar 6, 2015 at 11:54 AM, Nathan Bubna wrote:
> inline...
>
> On Fri, Mar 6, 2015 at 7:48 AM, Alex Fedotov wrote:
>
> > One more question. One of the other areas we making changes is around how
> > macros are called. Right now the lookup process is fairly rigid and can
> be
> > inefficient as well. Consider a couple of examples:
> >
> > 1. Scenario one - overload macro in the template This works now because
> the
> > lookup checks the namespace where the call is made first.
> >
> > macros.vtl
> >
> > #macro(B)
> > #end
> >
> > #macro(A)
> > #B()
> > #end
> >
> >
> > template.vtl
> > =
> > #parse('macros.vtl')
> > #macro(A)
> >## overloaded macro A
> > #end
> > #A() ## this calls local macro A which is expected.
> > =
> >
> > 2. Scenario two - overload macro B - this does not work now.
> >
> > template.vtl
> > =
> > #parse('macros.vtl')
> > #macro(B)
> >## overloaded macro B
> > #end
> > #A() ## this calls macro A which is then does NOT call overloaded B,
> since
> > the look-up would prefer the B from the same module where A is.
> > =
> >
> > We have created a custom directive #call which is similar to the
> > RuntimeMacro, but allows more control over the lookup process. This also
> > allows stuff like:
> > #call('self::macroName')
> > #call('nameSpace::macroName')
> >
> > The way every #parse(d) template is appended to the list of the
> > "macroLibraries" can be very bad for macro look-up speed:
> > #foreach( $item in $items )
> > #parse('item.vtl')
> > #end
> >
> > You end-up with a lot of "item.vtl" references added to the list which
> then
> > cause a lot of duplicate macro lookups in namespaces.
> > We have fixes that avoid adding duplicate sequences of template names to
> > the macroLibraries list.
> >
> > Do you guys have any plans to address this area?
> >
>
> So far as i know, no one has plans to work on these things.
>
>
> > We are also inclined to always use namespaces for macros, always allow
> > local macros to replace globals and keep the macros as ConcurrentHashMap
> in
> > the Template object.
> >
>
> Keeping macros in the Template would be good, but i'd be hesitant to accept
> a patch that permanently allows local macros to replace global macros.
> There are a LOT of Velocity users out there who use it to allow
> third-parties to write templates. Those Velocity users tend to like the
> ability to limit what their untrusted users can do with templates.
>
>
> > This way VelocimacroManager can be dropped all together, there is no
> issue
> > with reloading templates while maintaining the VelocimacroManager in
> sync,
> > less synchronization problems, etc.
> > The flag "isFromLib" for macros would also not be needed, so all the hair
> > around maintaining it during the "init" phase could be removed as well.
> >
>
> That sounds good. Can you do that while keeping the option to block local
> macros from overriding global ones?
>
>
>
> >
> > Alex
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > On Fri, Mar 6, 2015 at 2:48 AM, Claude Brisson
> wrote:
> >
> > > Hi.
> > >
> > > We'll publish the 2.0 one day, it's slowly moving...
> > >
> > > Thank you for your proposal. Of course, it is interesting for the
> project
> > > if you can share your 1.7 bugfixes as long as your synchronization work
> > on
> > > the 2.0 branch.
> > >
> > > The process is rather easy: patches are to be submitted on JIRA :
> > > https://issues.apache.org/jira/browse/VELOCITY/
> > >
> > > Very small patches (too small to be considered intellectual property)
> > > don't need anything else. For more consequent patches, some people say
> > you
> > > should at least add a sentence like "feel free to use this patch under
> > the
> > > Apache License"
Re: Velocity 2.0 (trunk) macro synchronization
There are some open questions though:
There is a very useful method Template.merge(Context, Writer, List
macroLibraries).
Should these macroLibraries be treated as global or local? I believe they
are treated as local now.
Alex
On Fri, Mar 6, 2015 at 12:08 PM, Alex Fedotov wrote:
> Sure it can be done.
>
> I would still keep the macros in templates, including the global templates
> (i.e. library files). I would maintain the list of global Template objects.
> Depending on the settings, during the macro lookup iterate over the global
> Template objects first, if not found there do the normal lookup in the
> calling template or in the Template list (macroLibraries).
>
> Alex
>
>
> On Fri, Mar 6, 2015 at 11:54 AM, Nathan Bubna wrote:
>
>> inline...
>>
>> On Fri, Mar 6, 2015 at 7:48 AM, Alex Fedotov wrote:
>>
>> > One more question. One of the other areas we making changes is around
>> how
>> > macros are called. Right now the lookup process is fairly rigid and can
>> be
>> > inefficient as well. Consider a couple of examples:
>> >
>> > 1. Scenario one - overload macro in the template This works now because
>> the
>> > lookup checks the namespace where the call is made first.
>> >
>> > macros.vtl
>> >
>> > #macro(B)
>> > #end
>> >
>> > #macro(A)
>> > #B()
>> > #end
>> >
>> >
>> > template.vtl
>> > =
>> > #parse('macros.vtl')
>> > #macro(A)
>> >## overloaded macro A
>> > #end
>> > #A() ## this calls local macro A which is expected.
>> > =
>> >
>> > 2. Scenario two - overload macro B - this does not work now.
>> >
>> > template.vtl
>> > =
>> > #parse('macros.vtl')
>> > #macro(B)
>> >## overloaded macro B
>> > #end
>> > #A() ## this calls macro A which is then does NOT call overloaded B,
>> since
>> > the look-up would prefer the B from the same module where A is.
>> > =
>> >
>> > We have created a custom directive #call which is similar to the
>> > RuntimeMacro, but allows more control over the lookup process. This also
>> > allows stuff like:
>> > #call('self::macroName')
>> > #call('nameSpace::macroName')
>> >
>> > The way every #parse(d) template is appended to the list of the
>> > "macroLibraries" can be very bad for macro look-up speed:
>> > #foreach( $item in $items )
>> > #parse('item.vtl')
>> > #end
>> >
>> > You end-up with a lot of "item.vtl" references added to the list which
>> then
>> > cause a lot of duplicate macro lookups in namespaces.
>> > We have fixes that avoid adding duplicate sequences of template names to
>> > the macroLibraries list.
>> >
>> > Do you guys have any plans to address this area?
>> >
>>
>> So far as i know, no one has plans to work on these things.
>>
>>
>> > We are also inclined to always use namespaces for macros, always allow
>> > local macros to replace globals and keep the macros as
>> ConcurrentHashMap in
>> > the Template object.
>> >
>>
>> Keeping macros in the Template would be good, but i'd be hesitant to
>> accept
>> a patch that permanently allows local macros to replace global macros.
>> There are a LOT of Velocity users out there who use it to allow
>> third-parties to write templates. Those Velocity users tend to like the
>> ability to limit what their untrusted users can do with templates.
>>
>>
>> > This way VelocimacroManager can be dropped all together, there is no
>> issue
>> > with reloading templates while maintaining the VelocimacroManager in
>> sync,
>> > less synchronization problems, etc.
>> > The flag "isFromLib" for macros would also not be needed, so all the
>> hair
>> > around maintaining it during the "init" phase could be removed as well.
>> >
>>
>> That sounds good. Can you do that while keeping the option to block local
>> macros from overriding global ones?
>>
>>
>>
>> >
>> > Alex
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> > On Fri, Mar 6, 2015 at 2:48 AM, Claude Brisson
>> wrote:
>> >
>> > > Hi.
>> > &g
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
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 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: [VOTE] 2.0 Release Quality
One more item that we had to fix is use of exceptions to control flow. Stuff like this: Parser$LookaheadSuccess : if (jj_la == 0 && jj_scanpos == jj_lastpos) throw jj_ls; I think there was something like that in VelocityCharStream as well. When creating exceptions for normal control flow "fillInStackTrace" becomes very expensive. Most of all it kills performance while debugger is attached. Alex On Wed, Nov 30, 2016 at 5:01 AM, Claude Brisson wrote: > I was delaying my vote for no real reason, but now I'm clearly in favor of > pushing out another RC with the following added: > > - include the Context in event parameters and deprecate ContextAware > interface (and all related interfaces and methods) > > - review ASTStringLiteral implementation to avoid unecessary string > buffering > > So, still > > [x] Leave at test build > > > Claude > > > On 18/11/2016 18:33, Nathan Bubna wrote: > >> +1 GA >> >> On Wed, Nov 16, 2016 at 7:24 AM, Greg Huber wrote: >> >> Thanks, works great for me. >>> >>> [x] General Availability (GA) >>> >>> +1 non binding. >>> >>> >>> On 16 November 2016 at 11:51, Claude Brisson wrote: >>> >>> The Velocity Engine 2.0 RC3 test build has been available since the 12th, and the RC4 (which only contains trivial fixes) has just been published. Release notes: * https://dist.apache.org/repos/dist/dev/velocity/velocity-eng ine/2.0/release-notes.html Distribution: * https://dist.apache.org/repos/dist/dev/velocity/velocity-eng ine/2.0/ Maven 2 staging repository: * https://repository.apache.org/content/repositories/orgapache velocity-1013 If you have had a chance to review the test build, please respond with a vote on its quality: [ ] Leave at test build [ ] Alpha [ ] Beta [ ] General Availability (GA) Everyone who has tested the RC3 or the RC4 is invited to vote. Votes by PMC members are considered binding. A vote passes if there are at least three binding +1s and more +1s than -1s. Claude - To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For additional commands, e-mail: dev-h...@velocity.apache.org > > - > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > For additional commands, e-mail: dev-h...@velocity.apache.org > >
Re: 2.0 question
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 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
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 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 > 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
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 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
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 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
>> 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-unsubscr...@velocity.apache.org
> For additional commands, e-mail: dev-h...@velocity.apache.org
>
>
Re: 2.0 question
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 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
>> 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
>>>> 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
>
Re: 2.0 question
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 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
>> 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
>>>> 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).
>>>>>>
>>>>>>
>>>>>>
>>>>>> =
>>>>>>
>>>>>>
Re: 2.0 question
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:
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 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
>> 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
>>>> 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.
>&
Re: Velocity truth (was: Re: [ANNOUNCE] Velocity Engine 2.0 RC6 test build available)
You guys should definitely leave a way of disabling the toString() conversion in boolean expressions. There are many places where people do null checks if #if($obj)...#end. Classes almost never return an empty string or null string from the toString call. Even worse some classes may use toString for debugging purposes and produce very long strings (including nested objects, etc.). In the code above that would be a huge inefficiency. Alex On Sat, Jan 28, 2017 at 2:06 PM, Nathan Bubna wrote: > Shame that i can't remember anymore all my reasons for wanting those > "getAs" lookups. Wondering why getAsNumber and getAsBoolean are here > too. Anyone else recall the use case? And assuming that i had good reason > (that did happen sometimes ), i wonder why i pushed for bucking the > "to()" convention. > > As for the literals, the thought of them being used in #if statements in a > template language is cringe-inducing. What's the use-case? Temporary > debugging hacks? If so, part of me thinks maybe only 'true' should even be > allowed. > > On Sat, Jan 28, 2017 at 7:15 AM, Claude Brisson > wrote: > > > > > > What is the problem? > > Velocity "truthiness": > >>> https://issues.apache.org/jira/browse/VELOCITY-692 > >>> > >>> It should definitely be part of 2.0. I missed it because the issue was > >>> closed, we should have opened a 2.0 one to remember it. > >>> > >> > >> Thats's the problem if a closed/resolved issue does not have an > >> assignee. You never know who handled it without reading the entire > >> thread. A ticket should always have an assignee if code has been > changed. > >> > >> > > Here's what had been specified by Nathan at the time (order is > meaningful, > > and falseness seems easier to specify than truth): > > > > $obj is null > > $obj is boolean false > > $obj returns false from getAsBoolean() (provided there is such a method) > > $obj is empty string (CharSequence w/length 0) > > $obj returns true from isEmpty() (provided there is such a method) > > $obj is array of length 0 > > $obj returns null from getAsString() (provided there is such a method) > > $obj returns empty string from getAsString() (provided there is such a > > method) > > $obj returns null from getAsNumber() (provided there is such a method) > > $obj returns 0 from length() or size() (provided there is such a method) > > $obj returns empty string from toString() (provided there is such a > method) > > > > Regarding this spec: > > - I'm not sure about getAsString() ; toString() is usually the standard > > way of getting the String representation and should be enough. > > - I'm not convinced by the fact that zero should be true. I hear > Nathan's > > point that for a display language, zero is as legitimate as any other > > number to be displayed. But it breaks the principle of least surprise, > > since each and every other language around, when not forbidding number > > towards boolean implicit conversion, consider zero as false. > > > > So I'd rather go with: > > > > $obj is null > > $obj is Boolean false > > $obj is Number zero (whatever Number variant) > > $obj returns false from getAsBoolean() (provided there is such a method) > > $obj is empty string (CharSequence w/length 0) > > $obj returns true from isEmpty() (provided there is such a method) > > $obj is array of length 0 > > $obj returns null from getAsNumber() (provided there is such a method) > > $obj returns 0 from length() or size() (provided there is such a method) > > $obj returns empty string from toString() (provided there is such a > method) > > > > Also, I noticed that Velocity weren't very consistent with literals. The > > only literal returning true is the 'true' literal. "foo" is false, > whereas > > it should be consistent with $foo containing "foo". > > > > Claude > > > > > > - > > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > > For additional commands, e-mail: dev-h...@velocity.apache.org > > > > >
Re: Velocity truth (was: Re: [ANNOUNCE] Velocity Engine 2.0 RC6 test build available)
I guess I was not clear enough. This option does indeed exist in 1.7. I just wanted to make sure it does not get dropped in 2.0 as part of the cleanup work. Alex On Mon, Jan 30, 2017 at 4:04 PM, Nathan Bubna wrote: > On Mon, Jan 30, 2017 at 12:51 PM, Nathan Bubna wrote: > > > On Sat, Jan 28, 2017 at 11:23 AM, Alex Fedotov wrote: > > > >> You guys should definitely leave a way of disabling the toString() > >> conversion in boolean expressions. > >> > > > > Seems reasonable, and also familiar; this may have been discussed before. > > > > Oh, yes, and built. I knew this was familiar. :) > > See http://velocity.apache.org/engine/2.0/configuration.html : > > directive.if.tostring.nullcheck = true > > > > > >> There are many places where people do null checks if #if($obj)...#end. > >> > >> Classes almost never return an empty string or null string from the > >> toString call. Even worse some classes may use toString for debugging > >> purposes and produce very long strings (including nested objects, > etc.). > >> In the code above that would be a huge inefficiency. > >> > > > > Yes, that's part of the motive to have this lookup chain. If there's a > > match higher up, toString() is avoided. Big deal for maps and arrays. > > > > > >> > >> Alex > >> > >> On Sat, Jan 28, 2017 at 2:06 PM, Nathan Bubna wrote: > >> > >> > Shame that i can't remember anymore all my reasons for wanting those > >> > "getAs" lookups. Wondering why getAsNumber and getAsBoolean are > >> here > >> > too. Anyone else recall the use case? And assuming that i had good > >> reason > >> > (that did happen sometimes ), i wonder why i pushed for bucking > >> the > >> > "to()" convention. > >> > > >> > As for the literals, the thought of them being used in #if statements > >> in a > >> > template language is cringe-inducing. What's the use-case? Temporary > >> > debugging hacks? If so, part of me thinks maybe only 'true' should > even > >> be > >> > allowed. > >> > > >> > On Sat, Jan 28, 2017 at 7:15 AM, Claude Brisson > >> > wrote: > >> > > >> > > > >> > > > >> > >>>> What is the problem? > >> > >>>> > >> > >>>> Velocity "truthiness": > >> > >>> https://issues.apache.org/jira/browse/VELOCITY-692 > >> > >>> > >> > >>> It should definitely be part of 2.0. I missed it because the issue > >> was > >> > >>> closed, we should have opened a 2.0 one to remember it. > >> > >>> > >> > >> > >> > >> Thats's the problem if a closed/resolved issue does not have an > >> > >> assignee. You never know who handled it without reading the entire > >> > >> thread. A ticket should always have an assignee if code has been > >> > changed. > >> > >> > >> > >> > >> > > Here's what had been specified by Nathan at the time (order is > >> > meaningful, > >> > > and falseness seems easier to specify than truth): > >> > > > >> > > $obj is null > >> > > $obj is boolean false > >> > > $obj returns false from getAsBoolean() (provided there is such a > >> method) > >> > > $obj is empty string (CharSequence w/length 0) > >> > > $obj returns true from isEmpty() (provided there is such a method) > >> > > $obj is array of length 0 > >> > > $obj returns null from getAsString() (provided there is such a > method) > >> > > $obj returns empty string from getAsString() (provided there is > such a > >> > > method) > >> > > $obj returns null from getAsNumber() (provided there is such a > method) > >> > > $obj returns 0 from length() or size() (provided there is such a > >> method) > >> > > $obj returns empty string from toString() (provided there is such a > >> > method) > >> > > > >> > > Regarding this spec: > >> > > - I'm not sure about getAsString() ; toString() is usually the > >> standard > >> > > way of getting the String representation and should be enough. > >> > > - I'
Re: Velocity truth
I would try to something similar to Javascript convention https://javascriptweblog.wordpress.com/2011/02/07/truth-equality-and-javascript/ substituting CharSequence instead of String (so that other empty String like objects i.e. StringBuilder would evaluate as false) and also treating empty arrays and collections as "false". Not sure if Duck typing for methods like isEmpty(), length() or size() is really necessary, could just support BooleanSupplier interface for custom conversions. All other cases would be covered by collections and arrays. Alex On Mon, Feb 6, 2017 at 10:48 AM, Christopher Schultz < ch...@christopherschultz.net> wrote: > Claude, > > On 1/28/17 10:15 AM, Claude Brisson wrote: > > Here's what had been specified by Nathan at the time (order is > > meaningful, and falseness seems easier to specify than truth): > > > > $obj is null > > $obj is boolean false > > $obj returns false from getAsBoolean() (provided there is such a method) > > $obj is empty string (CharSequence w/length 0) > > $obj returns true from isEmpty() (provided there is such a method) > > $obj is array of length 0 > > $obj returns null from getAsString() (provided there is such a method) > > $obj returns empty string from getAsString() (provided there is such a > > method) > > $obj returns null from getAsNumber() (provided there is such a method) > > $obj returns 0 from length() or size() (provided there is such a method) > > $obj returns empty string from toString() (provided there is such a > method) > > I *hate* that last one. A great use-case that ran us into OOMEs for a > while until I figured out what was going on: > > 1. SELECT [fields] FROM table > 2. Build ArrayList with e.g. User objects > 3. Build a user list in HTML from a Velocity template like this: > > #if($users) > > #foreach($user in $users) > ... > #end > > #else > No users found :() > #end > > This gives me horrible performance and an OOME when the list gets too > long, because the check for #if($users) truthiness converts the whole > list collection into a String (which takes forever) which can be huge > (which can cause OOME). > > I have now set the "directive.if.tostring.nullcheck=false" configuration > property (and written a set of wrapper classes around Collection classes > that throws an exception when toString is called, so things fail in > development) to avoid this, but also taken to using this check instead: > > #if($users.size() > 0) > > But this gets me a warning about the "size" method not existing on a > null object when the list is null. So I get junk in my logs when I do > things the hacky-way and I get performance problems and OOMEs when I do > things the "correct" way (at least, it looks totally correct). > > > Regarding this spec: > > - I'm not sure about getAsString() ; toString() is usually the standard > > way of getting the String representation and should be enough. > > - I'm not convinced by the fact that zero should be true. I hear > > Nathan's point that for a display language, zero is as legitimate as any > > other number to be displayed. But it breaks the principle of least > > surprise, since each and every other language around, when not > > forbidding number towards boolean implicit conversion, consider zero as > > false. > > > > So I'd rather go with: > > > > $obj is null > > $obj is Boolean false > > $obj is Number zero (whatever Number variant) > > For floating point values, does this have to be *zero*, or just close > enough to zero? > > > $obj returns false from getAsBoolean() (provided there is such a method) > > $obj is empty string (CharSequence w/length 0) > > $obj returns true from isEmpty() (provided there is such a method) > > $obj is array of length 0 > > $obj returns null from getAsNumber() (provided there is such a method) > > $obj returns 0 from length() or size() (provided there is such a method) > > $obj returns empty string from toString() (provided there is such a > method) > > > > Also, I noticed that Velocity weren't very consistent with literals. The > > only literal returning true is the 'true' literal. "foo" is false, > > whereas it should be consistent with $foo containing "foo". > > Can we maybe make an exception for Collections? Maybe for a Collection > (or array), we never call toString() on it? [].toString will always give > you garbage (which will be truthy) and Collection.toString() will also > likely give you garbage and it will also always be truthy unless it's > (a) null or (b) the Collection implements toString in a surprising way > relative to java.util Collections. > > -chris > >
Re: release is ready
Maven repo does not seem to show 2.0 for the overall velocity jar: http://repo1.maven.org/maven2/org/apache/velocity/velocity/ On Sun, Aug 6, 2017 at 2:01 PM, Claude Brisson wrote: > The maven artefact has been publish, the release is visible on apache > mirrors, and the website now shows engine 2.0 documentation. > > I think the release is ready to be announced on the users list (and other > channels?). > > Can some of you please check that I didn't forget anything important and > that links are correctly working? > > Thanks. > > Claude > > > > - > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > For additional commands, e-mail: dev-h...@velocity.apache.org > >
