Use array instead of Stack in SAXParser? about thymeleaf HOT 13 CLOSED

This is a bit of an oldie (I was looking through some of the older issues, see if I could do anything in the interim), but are you talking about the elementStack property here: https://github.com/thymeleaf/thymeleaf/blob/2.1-master/src/main/java/org/thymeleaf/templateparser/xmlsax/AbstractNonValidatingSAXTemplateParser.java#L305 ? Stack does extend Vector, so it is theoretically slower than most collection classes. I see you've tagged the issue against Thymeleaf 3.0, but I'd like to try tackle this one.

My plan would be to change elementStack to be of the java.util.Deque interface, java.util.ArrayDeque class, and then use only the stack-like methods to treat the deque as a stack. That could be a good first step to getting this thing away from using a synchronized collection.

Do we have a benchmark for proving that the change was worth it?

from thymeleaf.

Wow, someone brave enough to fight against the low-level parsing code in Thymeleaf!! I think I'm going to cry with joy :-):-)

Yes, that Stack is the thing to be removed. This class will require a heavy rewrite in 3.0, when we adopt attoparser, but anyway a structure like that will be probably needed even in the new handler, in order to contain the current stack of DOM elements.

And we have a benchmark, yes. The scripts are at https://github.com/thymeleaf/thymeleaf-tests, all those whose names start with "benchmark". There are "normal" benchmarks and "comparison" benchmarks. And also with and without Spring (note that Spring EL is much faster than OGNL).

The comparison scripts are oriented towards benchmarking two profiles defined in the pom.xml file. So you will probably be benchmarking a profile that points to the current "stable" version (2.1.2.RELEASE) against a profile that points to the current "snapshot" version (2.1.3-SNAPSHOT), which should contain your modifications. Unfortunately, this means you will need to perform an "mvn deploy" each time you modify something, so that the benchmark can donwload the snapshot .jar and use it.

Anyway, those scripts are a bit obscure, so please ask anything you might need.

from thymeleaf.

I think I had a look at this stuff a few months ago when I was trying to get AttoParser working with the Eclipse plugin, so it's shouldn't be that scary. I'll give it a look sometime this week.

from thymeleaf.

Arg, the comparison/benchmark scripts require a Unix-like environment. Note to self: take the work MacBook home one of these nights.

from thymeleaf.

Actually, I run them with CYGWIN on a Windows machine...

from thymeleaf.

Thought I had a Unix-like environment on my Windows machine (Git Bash which had MingW), but it doesn't have everything in it that your script needs. Will give Cygwin a try.

from thymeleaf.

Got it going, but I get a tonne of errors in the tests like this:

[...\thymeleaf-tests\target\test-classes\engine\features\link\link01.thtest-001][KO] Test FAILED: Actual result does not match expected result.
Obtained:
[<a href="/testing/url?a[0]=b">go</a>
<a href="/testing/base/url?a[0]=b">go</a>
<a h]
at line 1 col 4, but expected:
[<a href="/testing/url?a%5B0%5D=b">go</a>
<a href="/testing/base/url?a%5B0%5D=b">go<]
[test:end]

from thymeleaf.

Oh, I didn't receive a GitHub email notification for this last comment... sorry.

What profile are you using? This probably has to do with the modifications made for fixing #265 and #267, but it shouldn't be failing for you if you use the current 2.1.3-SNAPSHOT (you should actually be obtaining those a%5B0%5D)

from thymeleaf.

I was using the 2.1-spring4 profile, but I changed it to use 2.1.2.RELEASE so I could compare it against another profile that used 2.1.3-SNAPSHOT that contained my changes. I guess if I want to do that I should use the thymeleaf-tests repository with tests for the 2.1.2.RELEASE version? Otherwise, how can I compare my changes against the current 2.1.3-SNAPSHOT?

from thymeleaf.

Hmmm.... you're right, that is a limitation of our test layout. Given tests had to be modified in order to adapt to the fixes in #265, they won't work in 2.1.2. And if you go back to the version of the tests that worked for 2.1.2, it won't work for your snapshot :-)

I suppose the easiest fix for your needs are to locally remove the problematic tests from the list of tests being used for benchmarking. They are listed at these index files: https://github.com/thymeleaf/thymeleaf-tests/tree/master/src/test-common/resources/benchmark

from thymeleaf.

I removed those tests from the benchmark suite and was finally able to get it to run 2.1.2.RELEASE vs the 2.1.3-SNAPSHOT. The results are... inconclusive :| My machine isn't very good as a benchmark machine, and so the comparisons were all over the place: sometimes it was in favour of the change, sometimes it wasn't, usually favouring one or the other by 5%.

If the point of this issue is to just get us away from using a synchronized collection, then I'll be happy to push the change to 2.1-dev. Otherwise, we can just leave this alone.

from thymeleaf.

I think we can consider a +- 5% difference as a "no difference". Fluctuations are completely normal as operating systems will not give you the full attention of the CPU the whole time. And virtual machines (non-dedicated) aren't an option either, fluctuations are even bigger there...

Given this will be rewritten in 3.0, I see no reason for changing now if we are not going to obtain a performance improvement...

Thanks so much for the testing!

from thymeleaf.

This no longer applies with the new Thymeleaf 3.0 parsing mechanisms. See #390

from thymeleaf.