Skip to content
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

Avoid duplicated IFileInfo fetches when creating resources #1598

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

Conversation

HannesWell
Copy link
Member

@jukzi I assume this didn't show up in your recent performance analysis of the resources handling so I assume it won't have a big impact, but avoiding subsequent duplicated file-fetches is probably still beneficial.

Copy link
Contributor

github-actions bot commented Oct 26, 2024

Test Results

 1 758 files  ±0   1 758 suites  ±0   1h 31m 14s ⏱️ + 2m 1s
 4 170 tests ±0   4 148 ✅ ±0   22 💤 ±0  0 ❌ ±0 
13 107 runs  ±0  12 943 ✅ ±0  164 💤 ±0  0 ❌ ±0 

Results for commit 6bfdd8c. ± Comparison against base commit 93a2f4b.

♻️ This comment has been updated with latest results.

@iloveeclipse
Copy link
Member

Isn't that intended extra check for case insensitive file systems?

@HannesWell
Copy link
Member Author

Isn't that intended extra check for case insensitive file systems?

Where exactly? And how does it help to fetch the info of the same store twice?
Sorry if I missed that discussion.

@iloveeclipse
Copy link
Member

No, I just wondered that this extra check was behind extra "if" condition for case insensitive file systems. May be that is not a mistake but intention. I would check in git blame when it was added, for sure it is something Windows specific.

if (name != null && !store.getName().equals(name)) {
String msg = NLS.bind(Messages.resources_existsLocalDifferentCase, IPath.fromOSString(store.toString()).removeLastSegments(1).append(name).toOSString());
String msg = NLS.bind(Messages.resources_existsLocalDifferentCase,
Path.of(store.toString()).resolveSibling(name));
throw new ResourceException(IResourceStatus.CASE_VARIANT_EXISTS, getFullPath(), msg, null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CASE_VARIANT_EXISTS makes only sense when name is tested against the actual name in the filesystem. This codepath is only taken if localInfo.exists(), so never in the performance critical case where where the directory is clean.

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