Skip to content

Code cleanup, Patterns instead of operations direct on String #5546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kamilkrzywanski
Copy link

Code cleanup, Patterns instead of operations direct on String
enhanced loops, breaks in loops, use primitives, toArray with [0] initializer

enhanced loops, breaks in loops, use primitives, toArray with [0] initializer
@pizzi80
Copy link
Contributor

pizzi80 commented Feb 5, 2025

good job!

the toArray with [0] initializer to me it's not an optimization...
After some investigation I've disabled that suggestion on IntelliJ

If you inspect the code of the toArray(T[]) method you'll see that
if you pass a correct sized Array it will use directly the System.arraycopy method
instead of the Arrays.copyOf method which is optimized by JVM but it's still
less efficient of the native System.arraycopy call.

Probably at runtime the C2 compiler will optimize that but it requires some work,
so at best they will be on par but there is no advantage in first place if you know the size of the array

@kamilkrzywanski
Copy link
Author

@pizzi80 This change is indeed debatable in terms of its actual benefit. If you prefer, I can revert it.

@pizzi80
Copy link
Contributor

pizzi80 commented Feb 5, 2025

@pizzi80 This change is indeed debatable in terms of its actual benefit.

Yes!
I read many discussions also on S.O. ... I only reported my conclusions ;) ;)

If you prefer, I can revert it.

I suggest to wait the review of @BalusC because
it's the only person that can review and accept this kind of pull requests

I've done a lot of these optimizations in the past
and usually you've to wait a bit because there is a lot of code to review

In the meantime I've cherry picked some of your changes in my fork
so I'll test it to give my feedback

@BalusC
Copy link
Contributor

BalusC commented Feb 7, 2025

Thank you, but frankly it's better to break down each different kind of mass-change in a separate PR for a quicker mind processing and easier (isolated) approval/rejection.

Right now this is basically a big PR with a lot of unrelated cosmetic changes which do not fix real world problems.

On such a big code base it's generally better to rewrite/cleanup/modernize only the part when you actually need to work on that part.

@BalusC
Copy link
Contributor

BalusC commented Feb 7, 2025

And indeed, the [0] initializer is a bad idea. The existing code was completely fine. Though these days the list.toArray(Type[]::new) is considered the most efficient way, so the existing code is still open for improvement in this directrion ;)

@pizzi80
Copy link
Contributor

pizzi80 commented Feb 7, 2025

Sorry for the late reply,
I've ported all the changes in my fork ( except the toArray(...) changes )
and everything is working fine

During this porting I had the chance to improve a lot of other things,
but at the moment I can't create all the Pull requests
(probably something like 100 / 150 PRs)

@BalusC which could be the best moment to create all those PR?

On such a big code base it's generally better to rewrite/cleanup/modernize only the part when you actually need to work on that part.

true but questionable: a lot of these changes are very simple to do with IntelliJ,
and imho are a long term investment to create a better and cleaner code base ...

(some classes are unreadable with a modern IDE, also the programming style
is very error prone, probably derived from C programming style and Java 1.4 with no IDE support)

@kamilkrzywanski
Copy link
Author

+1 For me, the most important change was replacing String.replaceAll with precompiled patterns because it was annoying during debugging of my project. The rest of the changes I made along the way.

@kamilkrzywanski
Copy link
Author

@BalusC
The (Type[]::new) approach is not necessarily the best choice. It creates a new method reference that pollutes the metaspace, increases the class file size, and prolongs the warm-up phase, leading to worse performance while the code is still being interpreted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants