[GitHub] [commons-lang] sebbASF commented on pull request #565: [LANG-1576] refine StringUtils.chomp
sebbASF commented on pull request #565: URL: https://github.com/apache/commons-lang/pull/565#issuecomment-674532472 You are correct - I was forgetting that the characters could be missing. As to testing the length, yes I think we do need to test for various lengths. Longer substrings may take longer to create; if that is true it should show that the new code is better. 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
[GitHub] [commons-lang] sebbASF commented on pull request #565: [LANG-1576] refine StringUtils.chomp
sebbASF commented on pull request #565: URL: https://github.com/apache/commons-lang/pull/565#issuecomment-674371834 I think the current huge list of input strings is more about testing functionality rather than performance. The method only cares about 3 characters: CR, LF or something else (unless it has a bug). So the performance test needs to check those in various combinations. I think that is just 9 combinations. It also needs to test the length, because that is used when doing the substring. The method is likely to be used with textual input so it would make sense to try with a selection of lengths. Not sure what the maximum should be, probably at least 1000, maybe considerably more. It might make sense to do these as separate tests to see if the length affects the performance. 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
[GitHub] [commons-lang] sebbASF commented on pull request #565: [LANG-1576] refine StringUtils.chomp
sebbASF commented on pull request #565: URL: https://github.com/apache/commons-lang/pull/565#issuecomment-671828303 Sorry, I see now that the strings do have a mix of CR and LF endings (or neither). However, the strings are all of length 2. The strings are not representative of the likely use cases. As to the change itself, it looks fine, but IMO the benchmark needs some work. 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
[GitHub] [commons-lang] sebbASF commented on pull request #565: [LANG-1576] refine StringUtils.chomp
sebbASF commented on pull request #565: URL: https://github.com/apache/commons-lang/pull/565#issuecomment-671269648 Thanks - much easier to see what has been changed now. i.e. cache the string length, and don't use substring unless it is needed. I don't think the RandomStrings test is a fair benchmark. The behaviour of the method depends only on the last one or two characters (and the length). AFAICT the strings are not random and don't have a mix of line-endings. So I think the test only measures the efficiency of String.substring where nothing needs to be dropped. 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
[GitHub] [commons-lang] sebbASF commented on pull request #565: [LANG-1576] refine StringUtils.chomp
sebbASF commented on pull request #565: URL: https://github.com/apache/commons-lang/pull/565#issuecomment-664055647 The JMH results are very long and obscure the thread of the PR. Is it possible to post just the summary inline, with a link to the full results in an attachment? That would make it much easier to follow the PR. 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
[GitHub] [commons-lang] sebbASF commented on pull request #565: [LANG-1576] refine StringUtils.chomp
sebbASF commented on pull request #565: URL: https://github.com/apache/commons-lang/pull/565#issuecomment-664015190 If I understand the benchmark correctly, the New method is approx 3% slower for Test1 and about 2% faster for Test2. Does not seem like a huge gain. 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
