Re: 2.0 question

[Alex Fedotov]

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

[Alex Fedotov]

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: [VOTE] 2.0 Release Quality

[Alex Fedotov]

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

[Claude Brisson]

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-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org

Re: [VOTE] 2.0 Release Quality

[Claude Brisson]

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-engine/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

[tool] More tools reenginering

[Claude Brisson]

This time, it's about tools reading external resources. I'd like to:

1) have ImportSupport and ImportTool also be available in generic tools 
for absolute URLs, only the view versions being able to import a 
relative URL


2) have XmlTool inherit from ImportSupport, to homogenize its behavior 
with ImportTool


3) have a new generic JsonTool, in the same spirit

Example:

  #set($elements = $json.read("https://some.external.api/elements.json;))
  #foreach($element in $elements) ...

4) Put back XmlTool in the velocity-tools-generic module, using shading 
for the dom4j dependency. This is motivated by the fact that although 
dom4j-1.6.1.jar weights 446k, shading only needs 152k out of it. So it's 
a little loss for users not using the XmlTool, and a little gain for 
users using the XmlTool (see the exact figures in the appended annex). 
Not mentioning the gain in simplicity, one less jar...


5) have view versions of XmlTool and JsonTool, with the extra feature of 
being able to handle query POST parameters that are in text/json and 
text/xml format.


Example:

  javascript part:
  obj.foo = 'bar';
  $.post({
url: "some.template",
data: JSON.serialize(obj),
success: function(data) { do something },
error: function() { console.log("error"); },
contentType: "application/json"
  });

  template part in some.template:
  ... $json.post.foo ...

That's a typical client to server communication scheme. Of course, it 
certainly needs sanitization somewhere, as every client-side submission, 
but it's another story.


-

Annex: dom4j shading experiment features (which also shows some jar 
sizes evolution figures)


Here are the individual jar sizes:

(velocity-engine 1.5 383k)
---
velocity-engine 1.7 439k
dom4j-1.1 446k
velocity-tools 2.0 (generic) 189k
velocity-tools 2.0 (generic + view) 312k
---
velocity-engine 2.0 current RC 622k
velocity-tools-generic 3.0-SNAPSHOT 160k
velocity-tools-view 3.0-SNAPSHOT 99k
velocity-tools-xml 3.0-SNAPSHOT 14k
dom4j-1.6.1.jar 307k
velocity-tools-xml 3.0-SNAPSHOT with dom4j shading 166k

Here are Velocity + Tools configuration sizes:

Config engine 1.7 + generic tools 2.0 => 628k
Config engine 1.7 +view tools 2.0 => 751k

Config engine 1.7 + generic tools 2.0 + dom4j 1.1 => 1079k
Config engine 1.7 +view tools 2.0 + dom4j 1.1 => 1197k

Config engine 2.0 + generic tools 3.0   
=>  782k
Config engine 2.0 + generic tools & xml tool 3.0 + unshaded dom4j 1.6.1 
=> 1103k
Config engine 2.0 + generic tools & xml tool 3.0 + shaded dom4j 1.6.1 
=>  948k


Config engine 2.0 + view tools 3.0   =>  
881k
Config engine 2.0 + view tools & xml tool 3.0 + unshaded dom4j 1.6.1 => 
1202k

Config engine 2.0 + view tools & xml tool 3.0 + shaded dom4j 1.6.1 => 1047k

To put it in another way, with dom4j shading and XmlTool back into the 
generic tools (compared with the engine 2.0 + tools 3.0 config):
 - people not using the XmlTool will loose 166k, about +21% for generic 
tools users and +18% for view tools users
 - people using the XmlTool will gain  155k, about -14% for generic 
tools users and -13% for view tools users



  Claude


-
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org