Restore speed improvements #79

gnodet · 2022-05-02T08:20:58Z

No description provided.

src/main/java/org/codehaus/plexus/components/io/attributes/FileAttributes.java

src/main/java/org/codehaus/plexus/components/io/attributes/PlexusIoResourceAttributeUtils.java

src/main/java/org/codehaus/plexus/components/io/resources/PlexusIoFileResourceCollection.java

gnodet · 2022-05-17T07:17:38Z

@plamentotev would you mind having a quick look ?

plamentotev · 2022-05-17T18:35:11Z

@plamentotev would you mind having a quick look ?

I'll try to check it later this week or the beginning of the next. But I'm not completely sure what caused the original issue. And I wonder if there is simpler solution that keeping tract of the target using nested resource.

plamentotev · 2022-05-24T19:50:35Z

src/test/java/org/codehaus/plexus/components/io/resources/PlexusIoPlexusIoFileResourceTest.java

@@ -21,7 +21,7 @@ public void testRealSymlink()
        final File file = new File( "src/test/resources/symlinks/src/symDir" );
        PlexusIoResourceAttributes attrs = FileAttributes.uncached( file );
        assertTrue( attrs.isSymbolicLink() );
-        PlexusIoFileResource r = new PlexusIoFileResource( file, "symDir", attrs );


If my understanding is correct this change does not keep the current behavior of PlexusIoFileResource. If this is the case it should be a separate change (possibly requiring major version update if it is breaking change) as it is not just performance improvement.

I disagree. The PlexusIoFileResource has no public constructor, so the only way it can be created is through ResourceFactory.createResource or other internal methods.
What happens is that I think this change restores the original idea : PlexusIoSymlinkResource instances are used to represent symlinks and PlexusIoFileResource for regular files.

The problem with the initial commit, is that I missed the fact that attributes are handled in a very specific (and not necessarily intuitive way) for links : some attributes are retrieved from the link and some others from the target.
That's the reason why I've introduced the getLink() and getTarget() methods on the PlexusIoSymlinkResource which return PlexusIoResource instances that will more explicitly handle the link and the target.

Thanks for the explanation. Now it makes sense.

It would be nice if you can add some comments as for why getLink and getTarget are there to make it more clearer.

src/main/java/org/codehaus/plexus/components/io/resources/PlexusIoFileResource.java

src/main/java/org/codehaus/plexus/components/io/attributes/FileAttributes.java

sonatype-lift · 2022-05-29T21:42:39Z

src/main/java/org/codehaus/plexus/components/io/attributes/FileAttributes.java

+    /**
+     * @deprecated use {@link #FileAttributes(File)} and remove the unused userCache and groupCache parameters
+     */
+    @Deprecated
    public FileAttributes( @Nonnull File file, @Nonnull Map<Integer, String> userCache,


InlineMeSuggester: This deprecated API looks inlineable. If you'd like the body of the API to be inlined to its callers, please annotate it with @InlineMe.

Suggested change

public FileAttributes( @Nonnull File file, @Nonnull Map<Integer, String> userCache,

@InlineMe(replacement = "this(file)")

(at-me in a reply with help or ignore)

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

This reverts commit bf86860.

The PlexusIoFileResource constructor calls file.lastModified(), file.length(), file.isFile(), file.isDirectory() and file.exists() which is very inefficient

gnodet · 2022-05-30T16:08:39Z

I've pushed another commit which removes another set of file attributes access. We should be down to 2 calls per file, one with the follow links options and one without.

sonatype-lift · 2022-05-30T16:14:16Z

src/main/java/org/codehaus/plexus/components/io/resources/PlexusIoSymlinkResource.java


 public class PlexusIoSymlinkResource
    extends PlexusIoFileResource
    implements SymlinkDestinationSupplier
 {
-    private final File symnlinkDestination;
+    private final String symLinkDestination;


UnusedVariable: The field 'symLinkDestination' is never read.

(at-me in a reply with help or ignore)

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-05-30T16:14:17Z

src/main/java/org/codehaus/plexus/components/io/attributes/FileAttributes.java

@@ -39,6 +40,10 @@
 public class FileAttributes
    implements PlexusIoResourceAttributes
 {
+    public static final LinkOption[] FOLLOW_LINK_OPTIONS = new LinkOption[] { };
+
+    public static final LinkOption[] NOFOLLOW_LINK_OPTIONS = new LinkOption[] { LinkOption.NOFOLLOW_LINKS };


MutablePublicArray: Non-empty arrays are mutable, so this public static final array is not a constant and can be modified by clients of this class. Prefer an ImmutableList, or provide an accessor method that returns a defensive copy.

(at-me in a reply with help or ignore)

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

cstamas · 2022-06-22T09:56:04Z

Just a note: would be good if this PR goes in BEFORE this one #84 as it will most probably conflict (let me handle it) and it also removes Plexus completely, hence no more Junit3 tests as well...

src/main/java/org/codehaus/plexus/components/io/resources/PlexusIoFileResource.java

michael-o

LGTM. Squash, merge and move on.

wilx · 2022-09-15T10:51:35Z

I suspect this has caused https://issues.apache.org/jira/projects/MASSEMBLY/issues/MASSEMBLY-965. See also #89.

e-hubert · 2023-06-23T21:03:15Z

src/main/java/org/codehaus/plexus/components/io/resources/PlexusIoFileResourceCollection.java

        for ( String name : resources )
        {
            String sourceDir = name.replace( '\\', '/' );
            File f = new File( dir, sourceDir );

-            PlexusIoResourceAttributes attrs = new FileAttributes( f, cache1, cache2 );
-            attrs = mergeAttributes( attrs, f.isDirectory() );
+            FileAttributes fattrs = new FileAttributes( f );


This usage without the caches is now quite expensive and likely causing a performance regression reported in #109

sonatype-lift bot reviewed May 2, 2022

View reviewed changes

src/main/java/org/codehaus/plexus/components/io/attributes/FileAttributes.java Show resolved Hide resolved

sonatype-lift bot reviewed May 2, 2022

View reviewed changes

src/main/java/org/codehaus/plexus/components/io/attributes/PlexusIoResourceAttributeUtils.java Outdated Show resolved Hide resolved

sonatype-lift bot reviewed May 2, 2022

View reviewed changes

src/main/java/org/codehaus/plexus/components/io/resources/PlexusIoFileResourceCollection.java Show resolved Hide resolved

cstamas approved these changes May 17, 2022

View reviewed changes

plamentotev reviewed May 24, 2022

View reviewed changes

michael-o requested changes May 29, 2022

View reviewed changes

sonatype-lift bot reviewed May 29, 2022

View reviewed changes

michael-o self-requested a review May 29, 2022 21:49

gnodet added 5 commits May 30, 2022 18:05

Revert "Revert "Speed improvements (codehaus-plexus#33)""

d19f770

This reverts commit bf86860.

Deprecate constructor with unused parameters

274d04b

Correctly handle symlinks transparently

d7bbc79

Fix code style issues

89339a3

Cache another set of file attributes access

46245e4

The PlexusIoFileResource constructor calls file.lastModified(), file.length(), file.isFile(), file.isDirectory() and file.exists() which is very inefficient

gnodet force-pushed the restore-speed-improvements branch from 139a7e7 to 46245e4 Compare May 30, 2022 16:07

sonatype-lift bot reviewed May 30, 2022

View reviewed changes

michael-o requested changes Jun 22, 2022

View reviewed changes

src/main/java/org/codehaus/plexus/components/io/resources/PlexusIoFileResource.java Outdated Show resolved Hide resolved

olamy added the enhancement label Jun 22, 2022

Use Objects for requireNonNull

56f13b7

michael-o self-requested a review June 23, 2022 07:56

cstamas approved these changes Jun 23, 2022

View reviewed changes

michael-o approved these changes Jun 23, 2022

View reviewed changes

cstamas merged commit 6ffc902 into codehaus-plexus:master Jun 23, 2022

e-hubert mentioned this pull request Jun 23, 2023

Performance regression introduced with plexus-io 3.4.0 #109

Closed

e-hubert reviewed Jun 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore speed improvements #79

Restore speed improvements #79

gnodet commented May 2, 2022

gnodet commented May 17, 2022

plamentotev commented May 17, 2022

plamentotev May 24, 2022

gnodet May 30, 2022

gnodet May 30, 2022

plamentotev Jun 21, 2022

plamentotev Jun 21, 2022

sonatype-lift bot May 29, 2022

gnodet commented May 30, 2022

sonatype-lift bot May 30, 2022

sonatype-lift bot May 30, 2022

cstamas commented Jun 22, 2022 •

edited

Loading

michael-o left a comment

wilx commented Sep 15, 2022 •

edited

Loading

e-hubert Jun 23, 2023

	public FileAttributes( @Nonnull File file, @Nonnull Map<Integer, String> userCache,
	@InlineMe(replacement = "this(file)")

Restore speed improvements #79

Restore speed improvements #79

Conversation

gnodet commented May 2, 2022

gnodet commented May 17, 2022

plamentotev commented May 17, 2022

plamentotev May 24, 2022

Choose a reason for hiding this comment

gnodet May 30, 2022

Choose a reason for hiding this comment

gnodet May 30, 2022

Choose a reason for hiding this comment

plamentotev Jun 21, 2022

Choose a reason for hiding this comment

plamentotev Jun 21, 2022

Choose a reason for hiding this comment

sonatype-lift bot May 29, 2022

Choose a reason for hiding this comment

gnodet commented May 30, 2022

sonatype-lift bot May 30, 2022

Choose a reason for hiding this comment

sonatype-lift bot May 30, 2022

Choose a reason for hiding this comment

cstamas commented Jun 22, 2022 • edited Loading

michael-o left a comment

Choose a reason for hiding this comment

wilx commented Sep 15, 2022 • edited Loading

e-hubert Jun 23, 2023

Choose a reason for hiding this comment

cstamas commented Jun 22, 2022 •

edited

Loading

wilx commented Sep 15, 2022 •

edited

Loading