AbstractBundleContainer.java - Caching & Performance #2059
Conversation
laeubi
left a comment
laeubi
left a comment
There was a problem hiding this comment.
@vogella I don't think that such "modernization" currently bring us forward, if you absolute want this can you place make sure that:
- We can not cover it by cleanup action, if we can, enable them and let it happen automatically
- Batch other changes in one PR after cleanups where run.
I'm currently not looking into clean-up actions and have no plans to do so. Thanks for the suggestions but AFAIK according to the Eclipse policy you are not entitled to tell other committers how to work. |
|
I assume the build will fail (PDE build is currently not working), so I plan to leave this here and return to it, once PDE build are green again. |
You can work as you like, but as soon as you open a PR you need to accept to get it reviewed and probably there are some discussion, suggestions and best practices. For a while we have enabled cleanups to get a consistent codebase and to improve JDT leanups and you even agreed and asked it to rool out to platform, so it feel a bit odd now to complain. So if cleaning up the codebase is important to you I can only ask to follow what is established as it works on a larger scale and we are already familiar with that and it makes sure the same style is applied also in the future. |
HannesWell
left a comment
HannesWell
left a comment
There was a problem hiding this comment.
This performance optimazation seems useful.
But for my taste this adds too many comments and I also think the commit message isn't suitable.
Instead of listing the changes line-by-line it should give an high level overview over the change and/or explain the rational behind it.
The headline could also be more expressive, like
Speed-up vm-arguments lookup in AbstractBundleContainer or similar.
Due to this style I assume this change is AI generated, isn't it?
I know they are good at generating text, but I think we should still only add what's necessary and avoid what's not. Even comments have to be maintained.
| /** | ||
| * Cache for VM arguments to avoid repeated expensive resolution | ||
| */ |
There was a problem hiding this comment.
I think this comment is superflous and should be removed. IMHO comments only should be added if the code itself isn't clear or needs further explaination about its background.
In this case I think the use of this map is is obvious enough and I would just remove this comment.
The renmaing clearly helps to make the purpose obvious.
| if (entry.getKey().equals(this)) { | ||
| return entry.getValue(); | ||
| } | ||
| // Check cache first for faster lookup |
There was a problem hiding this comment.
This comment is IMO also superflous.
| // Check cache first for faster lookup |
|
A general statement: As much as automated optimizations/cleanups/modernizations/etc. are preferred, if someone feels like doing such a thing manually or via some other tool - It's equally welcome. We can make use of all the improvements in whatever way someone might decide to deliver them. |
Optimized the getVMArguments() method by replacing an inefficient O(n) iteration through all cache entries with a direct O(1) HashMap lookup. Also renamed the cache field from 'hash' to 'fVMArgsCache' for clarity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
vogella
force-pushed
the
abstractbundlecontainer
branch
from
644da33 to
52df5ee
Compare
|
Thanks @HannesWell update the PR according to your comments |
HannesWell
left a comment
HannesWell
left a comment
There was a problem hiding this comment.
Thanks for the update.
This looks good to me now.
| if (cachedArgs != null) { | ||
| return cachedArgs; | ||
| } | ||
|
|
There was a problem hiding this comment.
This extra blank line is IMO not necessary after a closing brace.
4 participants
Optimizations:
Impact: