[
https://issues.apache.org/jira/browse/TAP5-2332?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14005868#comment-14005868
]
Thiago H. de Paula Figueiredo commented on TAP5-2332:
-----------------------------------------------------
Jochen, you said: "I think we should try to make all parts of the code fast,
regardless of whether they are called often or just once in a while". I'm
sorry, but I need to be honest: what you said is wrong. Very wrong. Optimizing
code that is invoked just once in a while will have absolutely no improvement
in performance and all changes have a risk of introducing bugs in a mature
codebase. Correctness is more important then performance. It's a very known
rule about optimization that usually 80% of the running time is spent on just
20% of the code (of course, numbers vary). So, if you cut in half the running
time of the other 80%, of the code, you only get a 10% improvement. It's too
much work for little result. I think that's what Lance is saying. We should
only change something for performance if it'll actually make a difference.
IoCUtilities is the wrong place to put the concat() method because, as it's
name says, it's utilities for IoC. Cohesion! It would be very weird, not to say
wrong, to have concat() besides buildDefaultRegistry() and addDefaultModules().
Jochen, please don't commit code until we agree what needs to be optimized. We
have, so far, discussed a lot what needs to be changed, not how to concatenate
strings.
Jochen and Lance: send an e-mail to sa...@yourkit.com mentioning your
@apache.org e-mail address and they'll send you an YourKit Java profiler
license you can use to work on Tapestry. They answer quickly: I've asked
yesterday evening, got it today morning. By the way, thanks YourKit!
Again, request times are less important then knowing the hotspots so we only
change what will make a difference and leave code that won't make a difference
unchanged.
> Optimize String concatenation performance
> -----------------------------------------
>
> Key: TAP5-2332
> URL: https://issues.apache.org/jira/browse/TAP5-2332
> Project: Tapestry 5
> Issue Type: Improvement
> Reporter: Michael Mikhulya
> Assignee: Jochen Kemnade
> Priority: Minor
> Labels: performance
> Attachments: 0001-TAP5-2332-get-rid-of-String.format-usage.patch,
> profile-patched.png, profile-vanilla.png
>
>
> 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)
