On Fri, 27 Dec 2019 at 17:43, Matt Sicker <[email protected]> wrote: > It's somewhat logical, though, that T[0] is faster than T[size]. In > both cases, the given array is checked to see if it's large enough to > fit the collection contents. If it isn't, then a new array is > allocated. By deferring the new array allocation operation to the > toArray() method call, you avoid two array length checks. > > I don't think the size check is relevant.
AFAICT the reason modern JVMs are slower when passed T[size] is that the code no longer copies the collection directly into the provided array, but uses a temporary work array instead. The comments indicate that this is to protect against malicious multi-threaded code getting access to the collection. AFAIK the original JVM versions of the code did not have this protection. TL;DR: this is probably a spurious micro-optimization. > > Agreed. > On Fri, 27 Dec 2019 at 08:48, Gary Gregory <[email protected]> wrote: > > > > I wonder where this should be documented. We could add a // comment to > each > > call site, that seems the simplest. Adding a package or component level > > documentation in the style guideline for example could easily be > overlooked > > by someone in a debugger for example. So each call site would be best IMO > > something like > > // Using an empty array can be faster > > I don't really want a link to an external random blog in the source. > > > > Thoughts? > > > > Then we can consider doing it throughout Commons. > > > > Gary > > > > On Fri, Dec 27, 2019 at 7:23 AM Pascal Schumacher < > [email protected]> > > wrote: > > > > > Feel free to revert the changes. > > > > > > I would revert my changes, but Garry did some follow-up changes so > these > > > would to be reverted first. > > > > > > Am 27.12.2019 um 12:33 schrieb sebb: > > > > As that article says in conclusion, T[0] seems currently faster, but > that > > > > may not always be the case with future VMs. > > > > > > > > Also it says that IntelliJ IDEA and PMD recommend using T[size] > rather > > > than > > > > T[0]. > > > > If we release code with T[0], I suspect we will get complaints that > the > > > > code is not efficient according to x, y and z. > > > > > > > > I'm not convinced that the change is worth it, but if it is agreed > the > > > code > > > > must be thoroughly documented to try and forestall complaints. > > > > According to the article, using a constant empty array is faster, but > > > only > > > > marginally. If there is a constant available it would probably make > sense > > > > to use it. > > > > This should also be documented, as it affects the return value for > the > > > > empty case. > > > > > > > > S. > > > > On Fri, 27 Dec 2019 at 09:28, Pascal Schumacher < > > > [email protected]> > > > > wrote: > > > > > > > >> see https://shipilev.net/blog/2016/arrays-wisdom-ancients/ > > > >> > > > >> Am 27.12.2019 um 01:24 schrieb sebb: > > > >>> Also, where is it documented that modern JVMs are faster? > > > >>> To which JVMs does this apply? > > > >>> > > > >>> S. > > > >>> > > > >>> On Thu, 26 Dec 2019 at 22:08, Gary Gregory <[email protected] > > > > > >> wrote: > > > >>>> Please do not cause garbage to apparently be generated all over > the > > > >> place > > > >>>> by creating new empty arrays all the time. Use the constants > Commons > > > >> Lang > > > >>>> constants already defines; see ArrayUtils. > > > >>>> > > > >>>> Gary > > > >>>> > > > >>>> On Thu, Dec 26, 2019 at 4:48 PM <[email protected]> > wrote: > > > >>>> > > > >>>>> This is an automated email from the ASF dual-hosted git > repository. > > > >>>>> > > > >>>>> pascalschumacher pushed a commit to branch master > > > >>>>> in repository > https://gitbox.apache.org/repos/asf/commons-lang.git > > > >>>>> > > > >>>>> > > > >>>>> The following commit(s) were added to refs/heads/master by this > push: > > > >>>>> new 84668a2 Use Collection#toArray(new T[0]) instead of a > > > >> presized > > > >>>>> array as it is faster on modern JVMs. > > > >>>>> 84668a2 is described below > > > >>>>> > > > >>>>> commit 84668a2d980316a580030fd64764cb072b520b09 > > > >>>>> Author: pascalschumacher <[email protected]> > > > >>>>> AuthorDate: Thu Dec 26 22:48:12 2019 +0100 > > > >>>>> > > > >>>>> Use Collection#toArray(new T[0]) instead of a presized > array as > > > >> it is > > > >>>>> faster on modern JVMs. > > > >>>>> --- > > > >>>>> src/main/java/org/apache/commons/lang3/CharSet.java > | > > > 2 > > > >> +- > > > >>>>> src/main/java/org/apache/commons/lang3/StringUtils.java > | > > > 10 > > > >>>>> +++++----- > > > >>>>> .../org/apache/commons/lang3/exception/ExceptionUtils.java > | > > > 6 > > > >>>> +++--- > > > >>>>> > src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java | > > > 4 > > > >> ++-- > > > >>>>> .../java/org/apache/commons/lang3/reflect/MethodUtils.java > | > > > 2 > > > >> +- > > > >>>>> > src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java | > > > 2 > > > >> +- > > > >>>>> > src/main/java/org/apache/commons/lang3/text/StrTokenizer.java | > > > 4 > > > >> ++-- > > > >>>>> .../org/apache/commons/lang3/time/DurationFormatUtils.java > | > > > 2 > > > >> +- > > > >>>>> .../java/org/apache/commons/lang3/time/FastDatePrinter.java > | > > > 2 > > > >> +- > > > >>>>> > .../commons/lang3/concurrent/EventCountCircuitBreakerTest.java | > > > 2 > > > >> +- > > > >>>>> 10 files changed, 18 insertions(+), 18 deletions(-) > > > >>>>> > > > >>>>> diff --git a/src/main/java/org/apache/commons/lang3/CharSet.java > > > >>>>> b/src/main/java/org/apache/commons/lang3/CharSet.java > > > >>>>> index 3fdfd07..7955115 100644 > > > >>>>> --- a/src/main/java/org/apache/commons/lang3/CharSet.java > > > >>>>> +++ b/src/main/java/org/apache/commons/lang3/CharSet.java > > > >>>>> @@ -225,7 +225,7 @@ public class CharSet implements Serializable > { > > > >>>>> // NOTE: This is no longer public as CharRange is no longer a > > > public > > > >>>>> class. > > > >>>>> // It may be replaced when CharSet moves to Range. > > > >>>>> /*public*/ CharRange[] getCharRanges() { > > > >>>>> - return set.toArray(new CharRange[set.size()]); > > > >>>>> + return set.toArray(new CharRange[0]); > > > >>>>> } > > > >>>>> > > > >>>>> > > > >>>>> > > > >> > > > > //----------------------------------------------------------------------- > > > >>>>> diff --git > a/src/main/java/org/apache/commons/lang3/StringUtils.java > > > >>>>> b/src/main/java/org/apache/commons/lang3/StringUtils.java > > > >>>>> index abde7ec..d629806 100644 > > > >>>>> --- a/src/main/java/org/apache/commons/lang3/StringUtils.java > > > >>>>> +++ b/src/main/java/org/apache/commons/lang3/StringUtils.java > > > >>>>> @@ -7507,7 +7507,7 @@ public class StringUtils { > > > >>>>> currentType = type; > > > >>>>> } > > > >>>>> list.add(new String(c, tokenStart, c.length - > > > tokenStart)); > > > >>>>> - return list.toArray(new String[list.size()]); > > > >>>>> + return list.toArray(new String[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> @@ -7735,7 +7735,7 @@ public class StringUtils { > > > >>>>> } > > > >>>>> } > > > >>>>> > > > >>>>> - return substrings.toArray(new > String[substrings.size()]); > > > >>>>> + return substrings.toArray(new String[0]); > > > >>>>> } > > > >>>>> > > > >>>>> // > > > >>>>> > > > ----------------------------------------------------------------------- > > > >>>>> @@ -7923,7 +7923,7 @@ public class StringUtils { > > > >>>>> if (match || preserveAllTokens && lastMatch) { > > > >>>>> list.add(str.substring(start, i)); > > > >>>>> } > > > >>>>> - return list.toArray(new String[list.size()]); > > > >>>>> + return list.toArray(new String[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> @@ -8022,7 +8022,7 @@ public class StringUtils { > > > >>>>> if (match || preserveAllTokens && lastMatch) { > > > >>>>> list.add(str.substring(start, i)); > > > >>>>> } > > > >>>>> - return list.toArray(new String[list.size()]); > > > >>>>> + return list.toArray(new String[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> @@ -8835,7 +8835,7 @@ public class StringUtils { > > > >>>>> if (list.isEmpty()) { > > > >>>>> return null; > > > >>>>> } > > > >>>>> - return list.toArray(new String[list.size()]); > > > >>>>> + return list.toArray(new String[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> diff --git > > > >>>>> > > > a/src/main/java/org/apache/commons/lang3/exception/ExceptionUtils.java > > > >>>>> > > > b/src/main/java/org/apache/commons/lang3/exception/ExceptionUtils.java > > > >>>>> index 5969614..dd154e3 100644 > > > >>>>> --- > > > >>>> > a/src/main/java/org/apache/commons/lang3/exception/ExceptionUtils.java > > > >>>>> +++ > > > >>>> > b/src/main/java/org/apache/commons/lang3/exception/ExceptionUtils.java > > > >>>>> @@ -274,7 +274,7 @@ public class ExceptionUtils { > > > >>>>> } > > > >>>>> frames.addAll(trace); > > > >>>>> } > > > >>>>> - return frames.toArray(new String[frames.size()]); > > > >>>>> + return frames.toArray(new String[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> @@ -325,7 +325,7 @@ public class ExceptionUtils { > > > >>>>> while (frames.hasMoreTokens()) { > > > >>>>> list.add(frames.nextToken()); > > > >>>>> } > > > >>>>> - return list.toArray(new String[list.size()]); > > > >>>>> + return list.toArray(new String[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> @@ -438,7 +438,7 @@ public class ExceptionUtils { > > > >>>>> */ > > > >>>>> public static Throwable[] getThrowables(final Throwable > > > >> throwable) { > > > >>>>> final List<Throwable> list = > getThrowableList(throwable); > > > >>>>> - return list.toArray(new Throwable[list.size()]); > > > >>>>> + return list.toArray(new Throwable[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> diff --git > > > >>>>> a/src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java > > > >>>>> b/src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java > > > >>>>> index d72dc53..553be4e 100644 > > > >>>>> --- > a/src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java > > > >>>>> +++ > b/src/main/java/org/apache/commons/lang3/reflect/FieldUtils.java > > > >>>>> @@ -199,7 +199,7 @@ public class FieldUtils { > > > >>>>> */ > > > >>>>> public static Field[] getAllFields(final Class<?> cls) { > > > >>>>> final List<Field> allFieldsList = > getAllFieldsList(cls); > > > >>>>> - return allFieldsList.toArray(new > > > Field[allFieldsList.size()]); > > > >>>>> + return allFieldsList.toArray(new Field[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> @@ -237,7 +237,7 @@ public class FieldUtils { > > > >>>>> */ > > > >>>>> public static Field[] getFieldsWithAnnotation(final > Class<?> > > > cls, > > > >>>>> final Class<? extends Annotation> annotationCls) { > > > >>>>> final List<Field> annotatedFieldsList = > > > >>>>> getFieldsListWithAnnotation(cls, annotationCls); > > > >>>>> - return annotatedFieldsList.toArray(new > > > >>>>> Field[annotatedFieldsList.size()]); > > > >>>>> + return annotatedFieldsList.toArray(new Field[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> diff --git > > > >>>>> a/src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java > > > >>>>> b/src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java > > > >>>>> index bbd5019..491470d 100644 > > > >>>>> --- > a/src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java > > > >>>>> +++ > b/src/main/java/org/apache/commons/lang3/reflect/MethodUtils.java > > > >>>>> @@ -878,7 +878,7 @@ public class MethodUtils { > > > >>>>> final > boolean > > > >>>>> searchSupers, final boolean ignoreAccess) { > > > >>>>> final List<Method> annotatedMethodsList = > > > >>>>> getMethodsListWithAnnotation(cls, annotationCls, searchSupers, > > > >>>>> ignoreAccess); > > > >>>>> - return annotatedMethodsList.toArray(new > > > >>>>> Method[annotatedMethodsList.size()]); > > > >>>>> + return annotatedMethodsList.toArray(new Method[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> diff --git > > > >>>> a/src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java > > > >>>>> b/src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java > > > >>>>> index 2a6ccd0..f9df8c6 100644 > > > >>>>> --- > a/src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java > > > >>>>> +++ > b/src/main/java/org/apache/commons/lang3/reflect/TypeUtils.java > > > >>>>> @@ -1149,7 +1149,7 @@ public class TypeUtils { > > > >>>>> } > > > >>>>> } > > > >>>>> > > > >>>>> - return types.toArray(new Type[types.size()]); > > > >>>>> + return types.toArray(new Type[0]); > > > >>>>> } > > > >>>>> > > > >>>>> /** > > > >>>>> diff --git > > > >>>> a/src/main/java/org/apache/commons/lang3/text/StrTokenizer.java > > > >>>>> b/src/main/java/org/apache/commons/lang3/text/StrTokenizer.java > > > >>>>> index c9ab666..97fae7d 100644 > > > >>>>> --- > a/src/main/java/org/apache/commons/lang3/text/StrTokenizer.java > > > >>>>> +++ > b/src/main/java/org/apache/commons/lang3/text/StrTokenizer.java > > > >>>>> @@ -604,10 +604,10 @@ public class StrTokenizer implements > > > >>>>> ListIterator<String>, Cloneable { > > > >>>>> if (chars == null) { > > > >>>>> // still call tokenize as subclass may do some > > > work > > > >>>>> final List<String> split = tokenize(null, 0, > 0); > > > >>>>> - tokens = split.toArray(new > String[split.size()]); > > > >>>>> + tokens = split.toArray(new String[0]); > > > >>>>> } else { > > > >>>>> final List<String> split = tokenize(chars, 0, > > > >>>>> chars.length); > > > >>>>> - tokens = split.toArray(new > String[split.size()]); > > > >>>>> + tokens = split.toArray(new String[0]); > > > >>>>> } > > > >>>>> } > > > >>>>> } > > > >>>>> diff --git > > > >>>>> > > > a/src/main/java/org/apache/commons/lang3/time/DurationFormatUtils.java > > > >>>>> > > > b/src/main/java/org/apache/commons/lang3/time/DurationFormatUtils.java > > > >>>>> index 83af2e0..4a75237 100644 > > > >>>>> --- > > > >>>> > a/src/main/java/org/apache/commons/lang3/time/DurationFormatUtils.java > > > >>>>> +++ > > > >>>> > b/src/main/java/org/apache/commons/lang3/time/DurationFormatUtils.java > > > >>>>> @@ -564,7 +564,7 @@ public class DurationFormatUtils { > > > >>>>> if (inLiteral) { // i.e. we have not found the end of > the > > > >>>> literal > > > >>>>> throw new IllegalArgumentException("Unmatched > quote in > > > >>>>> format: " + format); > > > >>>>> } > > > >>>>> - return list.toArray(new Token[list.size()]); > > > >>>>> + return list.toArray(new Token[0]); > > > >>>>> } > > > >>>>> > > > >>>>> > > > >>>>> > > > >> > > > > //----------------------------------------------------------------------- > > > >>>>> diff --git > > > >>>>> > a/src/main/java/org/apache/commons/lang3/time/FastDatePrinter.java > > > >>>>> > b/src/main/java/org/apache/commons/lang3/time/FastDatePrinter.java > > > >>>>> index a5edb28..888d3fb 100644 > > > >>>>> --- > > > a/src/main/java/org/apache/commons/lang3/time/FastDatePrinter.java > > > >>>>> +++ > > > b/src/main/java/org/apache/commons/lang3/time/FastDatePrinter.java > > > >>>>> @@ -160,7 +160,7 @@ public class FastDatePrinter implements > > > >> DatePrinter, > > > >>>>> Serializable { > > > >>>>> */ > > > >>>>> private void init() { > > > >>>>> final List<Rule> rulesList = parsePattern(); > > > >>>>> - mRules = rulesList.toArray(new Rule[rulesList.size()]); > > > >>>>> + mRules = rulesList.toArray(new Rule[0]); > > > >>>>> > > > >>>>> int len = 0; > > > >>>>> for (int i=mRules.length; --i >= 0; ) { > > > >>>>> diff --git > > > >>>>> > > > >> > > > > a/src/test/java/org/apache/commons/lang3/concurrent/EventCountCircuitBreakerTest.java > > > >> > > > > b/src/test/java/org/apache/commons/lang3/concurrent/EventCountCircuitBreakerTest.java > > > >>>>> index b3fb5cf..2819006 100644 > > > >>>>> --- > > > >>>>> > > > >> > > > > a/src/test/java/org/apache/commons/lang3/concurrent/EventCountCircuitBreakerTest.java > > > >>>>> +++ > > > >>>>> > > > >> > > > > b/src/test/java/org/apache/commons/lang3/concurrent/EventCountCircuitBreakerTest.java > > > >>>>> @@ -407,7 +407,7 @@ public class EventCountCircuitBreakerTest { > > > >>>>> */ > > > >>>>> public void verify(final Boolean... values) { > > > >>>>> assertArrayEquals(values, > > > >>>>> - changedValues.toArray(new > > > >>>>> Boolean[changedValues.size()])); > > > >>>>> + changedValues.toArray(new Boolean[0])); > > > >>>>> } > > > >>>>> } > > > >>>>> } > > > >>>>> > > > >>>>> > > > >> > > > >> > --------------------------------------------------------------------- > > > >> To unsubscribe, e-mail: [email protected] > > > >> For additional commands, e-mail: [email protected] > > > >> > > > >> > > > > > > > > > --------------------------------------------------------------------- > > > To unsubscribe, e-mail: [email protected] > > > For additional commands, e-mail: [email protected] > > > > > > > > > > -- > Matt Sicker <[email protected]> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > >
