XenoAmess commented on a change in pull request #565: URL: https://github.com/apache/commons-lang/pull/565#discussion_r461489274
########## File path: src/test/java/org/apache/commons/lang3/StringUtilsChompTest.java ########## @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.lang3; + +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import static org.apache.commons.lang3.StringUtils.EMPTY; +import static org.apache.commons.lang3.StringUtils.isEmpty; +import static org.apache.commons.lang3.StringUtils.length; + +/** + * Test to show whether using BitSet for removeAll() methods is faster than using HashSet. + */ +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Thread) +public class StringUtilsChompTest { + + private static final String string1 = "\r"; + private static final String string2 = buildString2(); + + private static String buildString2() { + StringBuilder stringBuilder = new StringBuilder(); + for (int i = 0; i < 100000; i++) { + stringBuilder.append('0'); + stringBuilder.append('\r'); + stringBuilder.append('0'); + stringBuilder.append('\n'); + stringBuilder.append('0'); + stringBuilder.append('\r'); + stringBuilder.append('\n'); + } + return stringBuilder.toString(); + } + + @Benchmark + public void test1Old() { + chompOld(string1); Review comment: @sebbASF @thc202 thanks. remaked the jmh test. ########## File path: src/test/java/org/apache/commons/lang3/StringUtilsChompTest.java ########## @@ -0,0 +1,133 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.lang3; + +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import static org.apache.commons.lang3.StringUtils.EMPTY; +import static org.apache.commons.lang3.StringUtils.isEmpty; +import static org.apache.commons.lang3.StringUtils.length; + +/** + * Test to show whether using BitSet for removeAll() methods is faster than using HashSet. + */ +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Thread) +public class StringUtilsChompTest { + + private static final String string1 = "\r"; + private static final String string2 = buildString2(); + + private static String buildString2() { + StringBuilder stringBuilder = new StringBuilder(); + for (int i = 0; i < 100000; i++) { + stringBuilder.append('0'); + stringBuilder.append('\r'); + stringBuilder.append('0'); + stringBuilder.append('\n'); + stringBuilder.append('0'); + stringBuilder.append('\r'); + stringBuilder.append('\n'); + } + return stringBuilder.toString(); + } + + @Benchmark + public void test1Old() { + chompOld(string1); + } + + @Benchmark + public void test1New() { + chompNew(string1); + } + + @Benchmark + public void test2Old() { + chompOld(string2); Review comment: @aherbert yes it is. ########## File path: src/test/java/org/apache/commons/lang3/StringUtilsChompTest.java ########## @@ -0,0 +1,196 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.lang3; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.infra.Blackhole; + +import java.util.concurrent.TimeUnit; + +import static org.apache.commons.lang3.StringUtils.EMPTY; +import static org.apache.commons.lang3.StringUtils.isEmpty; +import static org.apache.commons.lang3.StringUtils.length; + +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Thread) +public class StringUtilsChompTest { + private static final String string1 = "\r"; + private static final String string2 = "a"; + private static final String string3 = "aa"; + private static final String string4 = "aa\r"; + private static final String string5 = "aa\n"; + private static final String string6 = buildString6(); + private static final String[] strings = buildStrings(); Review comment: > The majority of these strings will not end with either CR (char 13) or LF (char 10). So you will not be chomping anything most of the time. I do not think that is what you want to test the performance of. No, it is exactly what I test for. I use test 1-6 to show its performance with parameter who ends with \r or \n, and use this test s to show its performance with parameter who ends with other chars. ########## File path: src/test/java/org/apache/commons/lang3/StringUtilsChompTest.java ########## @@ -0,0 +1,196 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.lang3; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.infra.Blackhole; + +import java.util.concurrent.TimeUnit; + +import static org.apache.commons.lang3.StringUtils.EMPTY; +import static org.apache.commons.lang3.StringUtils.isEmpty; +import static org.apache.commons.lang3.StringUtils.length; + +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Thread) +public class StringUtilsChompTest { + private static final String string1 = "\r"; + private static final String string2 = "a"; + private static final String string3 = "aa"; + private static final String string4 = "aa\r"; + private static final String string5 = "aa\n"; + private static final String string6 = buildString6(); Review comment: > string6 will be the same as string5 my bad. I thought string5 would be "\r\n" and 6 be "\n", maybe I made a mistake. ########## File path: src/test/java/org/apache/commons/lang3/StringUtilsChompTest.java ########## @@ -0,0 +1,196 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.lang3; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.infra.Blackhole; + +import java.util.concurrent.TimeUnit; + +import static org.apache.commons.lang3.StringUtils.EMPTY; +import static org.apache.commons.lang3.StringUtils.isEmpty; +import static org.apache.commons.lang3.StringUtils.length; + +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Thread) +public class StringUtilsChompTest { Review comment: @aherbert I'm new to jmh and have no ideas what args should be set in normal test cases like this. Thanks for you guide. will retry with these settings. ########## File path: src/test/java/org/apache/commons/lang3/StringUtilsChompTest.java ########## @@ -0,0 +1,196 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.lang3; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.infra.Blackhole; + +import java.util.concurrent.TimeUnit; + +import static org.apache.commons.lang3.StringUtils.EMPTY; +import static org.apache.commons.lang3.StringUtils.isEmpty; +import static org.apache.commons.lang3.StringUtils.length; + +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Thread) +public class StringUtilsChompTest { + private static final String string1 = "\r"; + private static final String string2 = "a"; + private static final String string3 = "aa"; + private static final String string4 = "aa\r"; + private static final String string5 = "aa\n"; + private static final String string6 = buildString6(); + private static final String[] strings = buildStrings(); Review comment: > OK. Then you should comment each test to identify the intention, or better yet just name the test: > > ```java > public String test1Old() { > public String test1New() { > public String test2Old() { > public String test2New() { > ``` > > Becomes: > > ```java > public String test_CR_Old() { > public String test_CR_New() { > public String test_SingleChar_Old() { > public String test_SingleChar_New() { > ``` good idea. I will try to rename the tests. > From you latest results I would guess the 6% performance improvement for strings with nothing to chomp is probably due to not requiring a String.substring call to return a new instance. That has value. Yes, I think most reasons be in String.substring. but String.substring do not create a new String instance when it using (0, length) in cases like this. Still it have some checkings in that String.substring function, that might be what it cost the 6% ########## File path: src/test/java/org/apache/commons/lang3/StringUtilsChompTest.java ########## @@ -0,0 +1,196 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.lang3; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.infra.Blackhole; + +import java.util.concurrent.TimeUnit; + +import static org.apache.commons.lang3.StringUtils.EMPTY; +import static org.apache.commons.lang3.StringUtils.isEmpty; +import static org.apache.commons.lang3.StringUtils.length; + +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@State(Scope.Thread) +public class StringUtilsChompTest { + private static final String string1 = "\r"; + private static final String string2 = "a"; + private static final String string3 = "aa"; + private static final String string4 = "aa\r"; + private static final String string5 = "aa\n"; + private static final String string6 = buildString6(); + private static final String[] strings = buildStrings(); Review comment: @aherbert > OK. Then you should comment each test to identify the intention, or better yet just name the test: > > ```java > public String test1Old() { > public String test1New() { > public String test2Old() { > public String test2New() { > ``` > > Becomes: > > ```java > public String test_CR_Old() { > public String test_CR_New() { > public String test_SingleChar_Old() { > public String test_SingleChar_New() { > ``` good idea. I will try to rename the tests. > From you latest results I would guess the 6% performance improvement for strings with nothing to chomp is probably due to not requiring a String.substring call to return a new instance. That has value. Yes, I think most reasons be in String.substring. but String.substring do not create a new String instance when it using (0, length) in cases like this. Still it have some checkings in that String.substring function, that might be what it cost the 6% ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
