[
https://issues.apache.org/jira/browse/SOLR-13533?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16859925#comment-16859925
]
Uwe Schindler commented on SOLR-13533:
--------------------------------------
Hi,
I am fine with this patch, except for tests. They should not be "optimized"
(although it's no longer a required optimization, see below).
{quote}
So, something like:
StringBuilder sb = new StringBuilder();
sb.append("Hello, " + "World!");
Which defeats the very purpose of using a StringBuilder. And also it causes the
internal construction of extra StringBuilder objects by the JVM. And the JVM is
very conservative with that optimization. It does no interpretation, no
recognizing that there is already a StringBuilder present and adding it to that
one.
It is true that for simple concatentation, StringBuilder isn't required. But
when it is already being used, then using append for everything is the best
option.
{quote}
This is not quite true anymore. Lucene + Solr master is on Java 11 already, so
the class files are compiled for Java 11 and have target Java 11. This means,
simple string concats no longer use a StringBuilder internally. In fact, if you
concat a lot of stuff with static components like fragments and numbers, it's
now better to use string concats instead of StringBuilders. StringBuilders
should only be used for loops or if/then/else constructs.
Starting from Java 9, the "+" operator no longer uses StringBuilder and uses
instead the new feature "indyfied string concat" (which is BTW also used by
Elasticsearch in its painless scripting engine). What happens is that it
collects all static parts of the concat in to a static constant pool entry not
even passed to the method call, but part of the invokedynamic signature and the
remaining dynamic parts are pushed to stack as is. This allows to highly
optimize the string concats, which are only one single method call solely
containing the dynamic parts to a prepared MethodHandle that contains all
static parts.
So we should also review our string concats with StringBuilder and if they
don't use if/then/else and loops, replace them by "+".
> Code Cleanup - Performance
> --------------------------
>
> Key: SOLR-13533
> URL: https://issues.apache.org/jira/browse/SOLR-13533
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Reporter: Koen De Groote
> Priority: Trivial
> Labels: performance
>
> EDIT: Again, to clarify, please don't bother yourself with this ticket on
> company time, on personal time you could be working on something that makes
> you money or improves the product for your feature personally.
>
> This entire ticket is an afterthough. A look back at the code base that most
> people don't have the time for.
> ---------
>
> Code cleanup as suggested by static analysis tools. Will be done in my spare
> time.
> If someone reviews this, please also do not take up actual time from your
> work to do that. I do not wish to take away from your working hours.
>
> These are simple, trivial things, that were probably overlooked or not even
> considered(which isn't an accusation or something negative). But also stuff
> that the Java compiler/JIT won't optimize on its own.
>
> That's what static analysis tool are good for: picking stuff like that up.
>
> I'm talking about Intellij's static code analysis. Facebook's "Infer" for
> Java. Google's "errorprone", etc...
> These are the kinds of things that, frankly, for the people actually working
> on real features, are very time consuming, not even part of the feature, and
> have a very low chance of actually turning up a real performance issue.
> So I'm opting to have a look at the results of these tools and implementing
> the sensible stuff and if something bigger pops up I'll make a separate
> ticket for those things individually.
>
> Creating this ticket so I can name a branch after it.
>
> The only questions I have are: since the code base is so large, do I apply
> each subject to all parts of it? Or only core? How do I split it up?
> Do I make multiple PRs with this one ticket? Or do I make multiple tickets
> and give each their own PR?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
