You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by "paulirwin (via GitHub)" <gi...@apache.org> on 2024/12/05 23:48:47 UTC
[PR] SWEEP: Avoid unnecessary FileInfo/DirectoryInfo allocations, #832 [lucenenet]
paulirwin opened a new pull request, #1060:
URL: https://github.com/apache/lucenenet/pull/1060
- [X] You've read the [Contributor Guide](https://github.com/apache/lucenenet/blob/main/CONTRIBUTING.md) and [Code of Conduct](https://www.apache.org/foundation/policies/conduct.html).
- [X] You've included unit or integration tests for your change, where applicable.
- [X] You've included inline docs for your change, where applicable.
- [X] There's an open issue for the PR that you are making. If you'd like to propose a change, please [open an issue](https://github.com/apache/lucenenet/issues/new/choose) to discuss the change or find an existing issue.
Add string-based overloads to some methods that take FileInfo/DirectoryInfo to avoid unnecessary allocations
Fixes #832
## Description
See #832 for context. This makes changes to add and prefer string-based overloads for methods that take FileInfo and DirectoryInfo primarily just for the FullName property, to avoid unnecessary allocations.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Avoid unnecessary FileInfo/DirectoryInfo allocations, #832 [lucenenet]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1060:
URL: https://github.com/apache/lucenenet/pull/1060#discussion_r2286895555
##########
src/Lucene.Net/Store/SimpleFSLockFactory.cs:
##########
@@ -134,16 +134,16 @@ public override bool Obtain()
// LUCENENET: Since WriteAllText doesn't care if the file exists or not,
// we need to make that check first. We create a new IOException "failure reason"
// in this case to simulate what happens in Java
- if (File.Exists(lockFile.FullName))
+ if (File.Exists(lockFile))
{
- FailureReason = new IOException(string.Format("lockFile '{0}' alredy exists.", lockFile.FullName));
+ FailureReason = new IOException($"lockFile '{lockFile}' already exists.");
Review Comment:
The word 'alredy' in the original error message was corrected to 'already', which is good. However, the comment indicates this is to maintain consistency with the original typo, but the fix is appropriate.
##########
src/Lucene.Net.Benchmark/ByTask/Tasks/WriteLineDocTask.cs:
##########
@@ -112,8 +112,8 @@ public WriteLineDocTask(PerfRunData runData, bool performWriteHeader)
{
throw new ArgumentException("line.file.out must be set");
}
- Stream @out = StreamUtils.GetOutputStream(new FileInfo(m_fname));
- m_lineFileOut = new StreamWriter(@out, StandardCharsets.UTF_8);
+ Stream @out = StreamUtils.GetOutputStream(m_fname); // LUCENENET specific: changed to use string fileName instead of allocating a FileInfo (#832)
+ m_lineFileOut = new StreamWriter(@out, StandardCharsets.UTF_8); ;
Review Comment:
There is an extra semicolon at the end of the line. It should be removed.
```suggestion
m_lineFileOut = new StreamWriter(@out, StandardCharsets.UTF_8);
```
--
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.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Avoid unnecessary FileInfo/DirectoryInfo allocations, #832 [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin merged PR #1060:
URL: https://github.com/apache/lucenenet/pull/1060
--
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.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Avoid unnecessary FileInfo/DirectoryInfo allocations, #832 [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1060:
URL: https://github.com/apache/lucenenet/pull/1060#issuecomment-3259870127
I started a review, but got into the weeds a bit because I realized that the current port doesn't match the behavior of Java (at least according to ChatGPT - unconfirmed). The new string overload behavior should match Java, which is to pass the relative path down to the OS where it is resolved relative to the current application (the place where the jar lives). On the other hand, with `FileInfo` and `DirectoryInfo`, relative paths are resolved eagerly and not passed down to the OS. So, when we call `FullName` we don't necessarily have the same behavior.
Unfortunately, `FileInfo` and `DirectoryInfo` have no way to retrieve the original string that was passed into the constructor. So, there doesn't seem to be a reasonable fix,. Well, there is a protected `OriginalString` property that we could get to using the [`UnsafeAccessorAttribute`](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.unsafeaccessorattribute) on `net8.0`+, but we are probably better off accepting the differences of calling `FullName` in most cases.
Also, I suspect but haven't yet confirmed that the relative path behavior of our command line tools is broken. AFAIK, the expected behavior in a .NET console app is for a relative path to be resolved to the **environment's** current directory, but the behavior we have is relative to the directory of the executable. This behavior might be altered by wrapping these commands into a dotnet tool or by our (3rd party) wrapper project, though.
One other thing I noticed is that the `FileInfo.GetCanonicalPath()` method was not included here, probably because you didn't think to look for the common base class of `FileInfo` and `DirectoryInfo`: [`FileSystemInfo`](https://learn.microsoft.com/en-us/dotnet/api/system.io.filesysteminfo). This is the only place outside of the test framework and one test where we use it, though.
I will look into this more tomorrow. But we will need a clear idea of the *expected* behavior to get through this review.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Avoid unnecessary FileInfo/DirectoryInfo allocations, #832 [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1060:
URL: https://github.com/apache/lucenenet/pull/1060#issuecomment-3263420002
> I started a review, but got into the weeds a bit because I realized that the current port doesn't match the behavior of Java (at least according to ChatGPT - unconfirmed). The new string overload behavior should match Java, which is to pass the relative path down to the OS where it is resolved relative to the current application (the place where the jar lives).
I have analyzed this and determined that it largely behaves as you specify here: relative paths just work when passed down to the filesystem. Almost all .NET BCL methods that we use have predictable, correct behavior when relative paths are used. There is one exception, noted below.
| Function | Works with relative path? |
| --- | --- |
| Directory.Exists | Yes |
| FileStream constructor | Yes |
| Directory.GetParent | Yes |
| Directory.EnumerateFiles | Yes |
| File.Exists | Yes |
| FileInfo constructor | Yes |
| DirectoryInfo constructor | Yes |
| Path.GetDirectoryName | Somewhat* |
| Path.GetFileName | Yes |
| Directory.Delete | Yes |
| File.Delete | Yes |
| File.WriteAllText | Yes |
| Directory.CreateDirectory | Yes |
`Path.GetDirectoryName` (used here in a few places like Kuromoji, Benchmarks, and the CLI) does not itself error on a relative path, and works arguably correctly, but it returns a value that might not work correctly depending on how it was used previously. The function returns the directory component of a string containing a path, and does not resolve the directory of the path first. This means that for input values like `"."` and `".."`, it returns empty string (because those could be interpreted as the "file" component of a relative path, even though they have a special meaning). Then, for inputs like `"./foo.bar"`, it returns `"."`, because that is the directory component of the relative path. Methods like `WriteEnwikiLineDocTask.CategoriesLineFile` are fine with this behavior, because it's expected that you're passing in a full file name (and not a directory name). The only time when this is a problem is if you pass in those special relative directory names without a file name
, which could happen in CompoundFileExtractor. So I've fixed that by reverting that change and leaving a comment.
Note that only the Path class methods can have a problem, because the others actually hit the filesystem where the paths are resolved. Path just operates on strings without caring whether the path exists on disk. `Path.GetFileName` is not a problem because even if it's a completely slash-less path like `"foo.bar"` or any form of relative path to a file, the file name will extract correctly.
Note also that `~` for a home directory shorthand does not work correctly in most BCL types, including FileInfo and DirectoryInfo anyways, so I do not think we need to worry about that.
> On the other hand, with `FileInfo` and `DirectoryInfo`, relative paths are resolved eagerly and not passed down to the OS. So, when we call `FullName` we don't necessarily have the same behavior.
As noted above, this is fine in the cases in this PR.
> Unfortunately, `FileInfo` and `DirectoryInfo` have no way to retrieve the original string that was passed into the constructor. So, there doesn't seem to be a reasonable fix,. Well, there is a protected `OriginalString` property that we could get to using the [`UnsafeAccessorAttribute`](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.unsafeaccessorattribute) on `net8.0`+, but we are probably better off accepting the differences of calling `FullName` in most cases.
Yeah, I don't think we need to worry about this, since relative paths work for I/O.
> Also, I suspect but haven't yet confirmed that the relative path behavior of our command line tools is broken. AFAIK, the expected behavior in a .NET console app is for a relative path to be resolved to the **environment's** current directory, but the behavior we have is relative to the directory of the executable. This behavior might be altered by wrapping these commands into a dotnet tool or by our (3rd party) wrapper project, though.
I'm not sure we need to worry about this unless someone reports it. Seems like worst case you'd get a file/directory not found error, fix it, and move on.
> One other thing I noticed is that the `FileInfo.GetCanonicalPath()` method was not included here, probably because you didn't think to look for the common base class of `FileInfo` and `DirectoryInfo`: [`FileSystemInfo`](https://learn.microsoft.com/en-us/dotnet/api/system.io.filesysteminfo). This is the only place outside of the test framework and one test where we use it, though.
I did see this, but I don't think it's in scope (at least not for having a string-based overload). The method docs say that it returns the path "with all references resolved" and the easiest way to do that is to allocate a FileInfo/DirectoryInfo and calling `FullName` like it does anyways.
I'm not 100% convinced that this method truly needs to exist, but I am not going to worry about that at this time. (For example, using the lowercase path `"c:\\windows"` works fine in .NET operations with the lowercase drive letter.)
Now, there might be some possible analysis that could be done on whether or not code that _calls_ this method could avoid canonicalization and thus avoid FileInfo/DirectoryInfo... but that would be beyond the scope of just reducing allocations, as it possibly would result in non-canonical paths being used (assuming that is even important). I am not aware of a way to resolve a path without hitting the disk via I/O, and if you're doing that, the allocation of a FileInfo/DirectoryInfo is pretty negligible. The goal of this PR is only to help reduce allocations for cases where resolving the path is not necessary.
Also, it should be noted, _the original overloads are still there_. So if someone has a particular reason for using FileInfo/DirectoryInfo, they still can. This is not a breaking-change PR. So any new code that tries to use the string versions will need to confirm that it works for them with relative paths (it probably will), or just use an absolute path.
That said, I can add more unit tests to confirm this beyond my analysis and testing of the BCL. So I'll do that for what I can. Converting back to draft until I get that done. Feel free to review, though.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Avoid unnecessary FileInfo/DirectoryInfo allocations, #832 [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1060:
URL: https://github.com/apache/lucenenet/pull/1060#issuecomment-3224320396
Good points and analysis. How about we leave it as-is, and put comments on the string parameters to indicate that they are not normalized by this code? Then if we get any feedback about it in beta 18 we can consider revisiting it.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Avoid unnecessary FileInfo/DirectoryInfo allocations, #832 [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1060:
URL: https://github.com/apache/lucenenet/pull/1060#issuecomment-3222411253
Review from Claude Code:
## Pull Request Review: Issue #832 - Avoid Unnecessary FileInfo/DirectoryInfo Allocations
### **Summary**
This PR optimizes file system operations throughout the Lucene.NET codebase by replacing `FileInfo` and `DirectoryInfo` object allocations with direct string path operations where appropriate. This is a performance optimization that reduces memory allocations and GC pressure.
### **✅ Strengths**
1. **Consistent Performance Improvement**: The changes systematically replace temporary `FileInfo`/`DirectoryInfo` allocations with string paths, reducing object creation overhead.
2. **Proper Documentation**: All changes are well-documented with clear comments explaining the rationale (`// LUCENENET specific: changed to use string ... instead of allocating a FileInfo (#832)`).
3. **Backwards Compatibility**: The PR maintains existing public APIs by providing overloads, ensuring no breaking changes for consumers.
4. **Scope Appropriateness**: Changes are focused on internal implementations and areas where the objects were only used to get path strings.
### **⚠️ Areas of Concern**
1. **Redundant Method Chaining**: Several methods now just delegate to other overloads (e.g., in `HyphenationTree.cs` and `PatternParser.cs`). While this reduces allocations, it creates a chain of method calls that could be simplified.
2. **Missing Null Checks**: Some string path parameters don't appear to have explicit null validation where the original `FileInfo` parameters would have thrown `ArgumentNullException`.
3. **Potential Path Resolution Differences**: `FileInfo.FullName` normalizes paths differently than raw string paths. Need to ensure this doesn't introduce subtle behavioral changes.
### **🔍 Specific Observations**
1. **IOUtils.cs**: New overloads for `GetDecodingReader` methods properly handle string paths and reduce allocations.
2. **JaspellTernarySearchTrie.cs**: The expanded file reading logic with proper resource disposal is an improvement over the original implementation.
3. **Store Directory Classes**: Changes to `SimpleFSDirectory`, `NIOFSDirectory`, and `MMapDirectory` consistently use `Path.Combine` instead of creating `FileInfo` objects.
4. **Test File**: The test modification in `TestFilesystemResourceLoader.cs` correctly updates to match the new constructor signature.
### **📋 Recommendations**
1. **Add Null Validation**: Consider adding explicit null checks for string path parameters to match the behavior of `FileInfo` constructors.
2. **Consider Path Normalization**: Verify that switching from `FileInfo.FullName` to raw string paths doesn't cause issues with relative paths or path separators.
3. **Performance Testing**: Run benchmarks to quantify the performance improvements, especially in scenarios with high file I/O operations.
4. **Documentation Update**: Update XML documentation for methods that now accept string paths to clarify any behavioral differences.
### **Verdict: ✅ APPROVED with minor suggestions**
This PR successfully addresses the goal of reducing unnecessary allocations while maintaining API compatibility. The changes are well-documented and follow a consistent pattern. The minor suggestions above would further improve robustness but are not blocking issues.
---
As a result of those suggestions, I did enable nullable reference type checking on StopAnalyzer and StopwordAnalyzerBase to resolve that concern. "Redundant method chaining" is to keep with matching upstream code in many cases, and "Potential path resolution differences" does not seem to have much of an impact for the cases that CC came up with (i.e. mixing slashes).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Avoid unnecessary FileInfo/DirectoryInfo allocations, #832 [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1060:
URL: https://github.com/apache/lucenenet/pull/1060#issuecomment-3256985482
This old PR has been rebased against latest main and feedback has been addressed. I intend to merge this PR this coming weekend if there is no further feedback.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org