Upgrade HtmlFlow to release 3.5

fmcarvalho commented 4 years ago

Fixed a problem where the block html was written two times into the response. Reported by @Vest at https://github.com/jreijn/spring-comparing-template-engines/issues/34#issuecomment-490099263
Updated the HtmlFlowIndexView according to the new behavior of HtmlFlow in version 3.3. (https://github.com/xmlet/HtmlFlow#33-august-2019) where it: ”disallows the use of chained calls to dynamic() due to unexpected cache behaviors.”. Thus we had to replace the inner invocations of dynamic() by of().
Changed the static method getView() of HtmlFlowIndexView with a static field view.
Added the threadSafe() initialization to the previous view to enable safe use on concurrent benchmarks.

Vest commented 4 years ago

@fmcarvalho I didn't have time to review your new PR. I found out that you replaced the public function getView with the static variable view. So it means that comparing to other engines, that we were using in this repository, your view is not recreated on every test iteration.

I debugged your code for quite a long time and I found out that with this change, the shared DynamicHtml object uses HtmlVisitorCache, where the property isCached is equal to true for every thread that is created by Spring.

E.g. if you put the breakpoint here:

// HtmlFlowView.renderMergedTemplateModel
String html = HtmlFlowIndexView.view.render(model);

and step into

// DynamicHtml.render
public final String render(T model) {
    binder.accept(this, model);
    return getVisitor().finished(); // <-- here
}

you will find that the method finished reads cached parts of the dynamic content from cacheBlockList. See the image below:

Maybe I am wrong, but I guess that what happens is unfair to other engines, where the view is built and parsed at runtime for each request individually. In your case, we just take strings from the cache.

I will return my change back, but leave your PR with the new version of HtmlFlow.

@jreijn if you are interested in this finding, you can test the PR as well.

fmcarvalho commented 4 years ago

@Vest You are not right when you said: "the method finished reads cached parts of the dynamic content from cacheBlockList.". It only reads the static parts and not the dynamic parts.

As you can see in your print screen you only have 2 blocks in cacheBlockList the prologue that begins with <DOCTYPE html>\n<html>... and the epilogue with \n\t\t</div>\n\t\t\<script... all the rest in the middle is dynamically generated has expressed through the call to dynamic() here and it is not in cache.

You can observe it by passing different models to this view and you will see different outputs.

The same approach happens with textual templates engines. The static parts are only concatenated with the dynamic parts. For example in JSP we do: "<%@ include file="head.jspf" %>" where head.jspf has an equivalent HTML definition to that one preceding the call to dynamic() in HtmlFlowIndexView.

Yet, in HtmlFlow since the static parts are defined through functions, we must first invoke those functions to get the output of the static part, that is in turn stored in the cacheBlockList.

Maybe the name cacheBlockList generates some confusing here and should be better called staticBlockParts or something like that.

Vest commented 4 years ago

yes, it seems that I was wrong. I apologize. This name was really confusing, because I was trying to understand what kind of data is cached and why by the time when the rendering starts, the string builder is initialized with the data.

Ok, it seems fine, or at least I didn't find anything suspicious.

Thanks

fmcarvalho commented 4 years ago

@Vest We are already using the term static in some code of this class such as staticBlockIndex. Although the HtmlVisitorCache is public it plays an internal role inside HtmlFlow and I will rename it and all its related members from cache to something that suppresses that ambiguity and misleadings. Names should clear express their semantics and here we are creating unnecessary confusing.

BTW when I am running the benchmarks I am getting slightly different results from those ones published in benchmarks-102019. I know those results depend of many different environment characteristics and hardware. But, I cannot observer JSP and Freemaker being the top two most performant. And I cannot observe either the JSP gaining for almost 3 or 4 seconds above competence.

Vest commented 4 years ago

I can repeat the tests tomorrow just for your curiosity, but indeed these results are not 100% precise. Sometimes they are 73% reproducible :) It is difficult to sort them by time and choose the fastest. Second more or less and you get a different picture.

fmcarvalho commented 4 years ago

@Vest did you have the opportunity to run the tests again? Did you see differences in results?

btw I forgot to update the htmlflow version in README.md to 3.5.

jreijn / spring-comparing-template-engines

Upgrade HtmlFlow to release 3.5 #56