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.

Sign up for GitHub

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

Jump to bottom

[performance] IFile.create: reduce 1 of 3 store.fetchInfo() #1445

Merged
merged 1 commit into from
Jun 26, 2024
Merged

[performance] IFile.create: reduce 1 of 3 store.fetchInfo() #1445

merged 1 commit into from
Jun 26, 2024

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Jun 25, 2024

Assume the file does not exist (normal case) - otherwise implementation fails later during actual write.

#1443

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 25, 2024 •
edited
Loading

Test Results

 1 734 files  ±0   1 734 suites  ±0   1h 34m 54s ⏱️ + 3m 32s
 3 972 tests ±0   3 950 ✅ +1   22 💤 ±0  0 ❌  - 1 
12 513 runs  ±0  12 352 ✅ +1  161 💤 ±0  0 ❌  - 1 

Results for commit f779562. ± Comparison against base commit b512a65.

♻️ This comment has been updated with latest results.

laeubi
laeubi reviewed Jun 25, 2024
View reviewed changes
resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/File.java
// under this location.
message = NLS.bind(Messages.resources_existsLocalDifferentCase,
IPath.fromOSString(store.toString()).removeLastSegments(1).append(name).toOSString());
throw new ResourceException(IResourceStatus.CASE_VARIANT_EXISTS, getFullPath(), message, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the error is no longer thrown it should be removed from the messages file(s) as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what exactly do you think is unused now?
image
image

@jukzi jukzi force-pushed the File_create_no_exist_check branch from c46b47a to cdb68f5 Compare June 25, 2024 13:46
@jukzi
Copy link
Contributor Author

jukzi commented Jun 25, 2024

rebased. expecting error for
".LocalOnlyFileHidden" at org.eclipse.core.tests.resources.IFileTest.testCreate(IFileTest.java:464)

@jukzi
Copy link
Contributor Author

jukzi commented Jun 25, 2024

As expected error on windows only:

failure in invocation 0 with inputs [L/OpenProject/ExistingFolder/.LocalOnlyFileHidden, java.io.ByteArrayInputStream@6ce7d6ab, true, org.eclipse.core.tests.harness.FussyProgressMonitor@53a3ccc]
invocation should succeed but unexpected exception occurred: org.eclipse.core.runtime.CoreException: Could not write file: D:\a\eclipse.platform\eclipse.platform\resources\tests\org.eclipse.core.tests.resources\target\work\data\OpenProject\ExistingFolder\.LocalOnlyFileHidden.
java.lang.AssertionError: 
failure in invocation 0 with inputs [L/OpenProject/ExistingFolder/.LocalOnlyFileHidden, java.io.ByteArrayInputStream@6ce7d6ab, true, org.eclipse.core.tests.harness.FussyProgressMonitor@53a3ccc]
invocation should succeed but unexpected exception occurred: org.eclipse.core.runtime.CoreException: Could not write file: D:\a\eclipse.platform\eclipse.platform\resources\tests\org.eclipse.core.tests.resources\target\work\data\OpenProject\ExistingFolder\.LocalOnlyFileHidden.
	at org.eclipse.core.tests.resources.TestPerformer.performTestRecursiveLoop(TestPerformer.java:96)
	at org.eclipse.core.tests.resources.TestPerformer.performTestRecursiveLoop(TestPerformer.java:111)
	at org.eclipse.core.tests.resources.TestPerformer.performTestRecursiveLoop(TestPerformer.java:111)
	at org.eclipse.core.tests.resources.TestPerformer.performTestRecursiveLoop(TestPerformer.java:111)
	at org.eclipse.core.tests.resources.TestPerformer.performTest(TestPerformer.java:58)
	at org.eclipse.core.tests.resources.IFileTest.testCreate(IFileTest.java:464)
Caused by: org.eclipse.core.runtime.CoreException: Could not write file: D:\a\eclipse.platform\eclipse.platform\resources\tests\org.eclipse.core.tests.resources\target\work\data\OpenProject\ExistingFolder\.LocalOnlyFileHidden.
	at org.eclipse.core.internal.filesystem.Policy.error(Policy.java:48)
	at org.eclipse.core.internal.filesystem.local.LocalFile.openOutputStream(LocalFile.java:507)
	at org.eclipse.core.internal.localstore.FileSystemResourceManager.write(FileSystemResourceManager.java:1227)
	at org.eclipse.core.internal.resources.File.internalSetContents(File.java:381)
	at org.eclipse.core.internal.resources.File.create(File.java:137)
	at org.eclipse.core.internal.resources.File.create(File.java:165)
	at org.eclipse.core.tests.resources.IFileTest$1.invokeMethod(IFileTest.java:430)
	at org.eclipse.core.tests.resources.TestPerformer.performTestRecursiveLoop(TestPerformer.java:90)
	... 97 more
Caused by: java.io.FileNotFoundException: D:\a\eclipse.platform\eclipse.platform\resources\tests\org.eclipse.core.tests.resources\target\work\data\OpenProject\ExistingFolder\.LocalOnlyFileHidden (Access is denied)
	at java.base/java.io.FileOutputStream.open0(Native Method)
	at java.base/java.io.FileOutputStream.open(FileOutputStream.java:289)
	at java.base/java.io.FileOutputStream.<init>(FileOutputStream.java:230)
	at org.eclipse.core.internal.filesystem.local.LocalFile.openOutputStream(LocalFile.java:497)
	... 103 more

@EcljpseB0T
[performance] IFile.create: reduce 1 of 3 store.fetchInfo()
f779562 
Assume the file does not exist (normal case) - otherwise implementation
fails later during actual write.

eclipse-platform#1443
@jukzi jukzi force-pushed the File_create_no_exist_check branch from cdb68f5 to f779562 Compare June 25, 2024 14:18
@jukzi
Copy link
Contributor Author

jukzi commented Jun 25, 2024

updated with fix for hidden files.

@jukzi
Copy link
Contributor Author

jukzi commented Jun 25, 2024

if there are no objections i plan to merge tomorrow

@HannesWell
Copy link
Member

Similar remark as in #1446 (comment).
Bottom line: We should be sure that this is a net improvement.

@jukzi
Copy link
Contributor Author

jukzi commented Jun 26, 2024

Similar remark as in #1446 (comment).
Bottom line: We should be sure that this is a net improvement.

Same reasoning as there: put a breakpoint in the catch clause and verify it never happens under normal conditions. It would only happen under the absurd conditions of:

  • windows (likely)
  • file is not up-to-date in workspace (only happens if resource API is not used)
  • file is hidden (never set by eclipse ide)

@jukzi jukzi merged commit 31f4f98 into eclipse-platform:master Jun 26, 2024
16 checks passed
@jukzi jukzi deleted the File_create_no_exist_check branch June 26, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laeubi laeubi laeubi left review comments

Assignees
No one assigned
Labels
performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jukzi @HannesWell @laeubi @EcljpseB0T