[
https://issues.apache.org/jira/browse/TAP5-2332?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14005059#comment-14005059
]
Thiago H. de Paula Figueiredo commented on TAP5-2332:
-----------------------------------------------------
My comments about this:
1) We want Tapestry to be as fast as possible.
2) We don't want to lose flexibility.
3) Reading the ticket title, "Get rid of String.format usage", I can say: that
will never happen. It's crucial for formatting user-facing messages that come
from property files and can be overridden by the developer. So, unless the
original intention and title of the ticket is changed, it's fair to close it as
"invalid" or "won't fix".
4) Of course, we can replace places which use String.format just for static
concatenation of Strings. But we need to know which ones will actually make a
difference. For example, the patch includes a change to
OptionModelImpl.toString(), which isn't used by the framework at all. It'll
just be invoked if you're debugging code or call it directly in your code.
5) The proposed concat() method is a nice idea, but putting it on IoCUtilities
isn't, because concat() is completely unrelated to IoC itself.
6) Michael, you've just posted end-to-end results (i.e. page rendering times).
What I wanted to see, which is probably also what Lance wants, is to see the
source code of the tested page (so we can better isolate framework code from
everything else) plus how much time (in %) was spent on the lines of code that
are the slowest. We need to know what the slowest are and optimize them. Just
replacing all String.format() calls we can is premature optimization indeed.
Michael, you mentioned some specific lines of code, but no numbers were
provided about them. Without them, we have premature optimization. With them,
we have good optimization.
7) Michael, which profiler are you using?
> Get rid of String.format usage
> ------------------------------
>
> Key: TAP5-2332
> URL: https://issues.apache.org/jira/browse/TAP5-2332
> Project: Tapestry 5
> Issue Type: Improvement
> Reporter: Michael Mikhulya
> Priority: Minor
> Labels: performance
> Attachments: 0001-TAP5-2332-get-rid-of-String.format-usage.patch
>
>
> During profiling I found that String.format provides much load on CPU.
> In many cases in Tapestry String.format can be easily replaced with simple
> String concatenation.
> Simple JMH (http://openjdk.java.net/projects/code-tools/jmh/) test
> {code:java}
> public class FormatVsConcat {
> private static final String format = "This is a test string with %s";
> private static final String concat1 = "This is a test string with ";
> private static final String concat2 = "test word";
> @GenerateMicroBenchmark
> public String format() {
> return String.format(format, concat2);
> }
> @GenerateMicroBenchmark
> public String concat() {
> return concat1 + concat2;
> }
> }
> {code}
> shows, that concatenation is 366(!) times faster.
> I removed only hot places in tapestry and get following results with apache
> benchmark:
> *Not patched* tapestry version:
> Requests per second: *21.38 /sec* (mean)
> Time per request: *46.764 [ms]* (mean)
> *Patched* tapestry version:
> Requests per second: *27.77 /sec* (mean)
> Time per request: *36.013 [ms]* (mean)
> So we gained 10ms per request or 20% of rendering time.
> If you don't mind I would like to get rid of String.format in all places of
> Tapestry and provide patch. I fixed only hot places which appeared during
> ab-profiling of one concrete page. So it is very likely that not all hot
> places were found and fixed.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
