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

Speed up project discovery #242

Closed
wants to merge 4 commits into from
Closed

Speed up project discovery #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57718ed
[MNG-6629] - Make ID validation faster
oehme Apr 9, 2019
46117fe
[MNG-6630] - Make ComparableVersion faster
oehme Apr 9, 2019
06065b0
[MNG-6631] - Make DefaultArtifactVersion faster
oehme Apr 9, 2019
6fc5267
[MNG-6632] - Remember artifact handlers after they've been used once
oehme Apr 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 100 additions & 4 deletions maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,30 @@ public int compareTo( Item item )
}
}

@Override
public boolean equals( Object o )
Copy link
Member

Choose a reason for hiding this comment

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

Why not use Objects#equals() on this?

Copy link
Contributor Author

@oehme oehme Apr 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Because it wouldn't save any code here, as nothing is nullable.

{
if ( this == o )
{
return true;
}
if ( o == null || getClass() != o.getClass() )
{
return false;
}

IntItem intItem = (IntItem) o;

return value == intItem.value;

}

@Override
public int hashCode()
{
return value;
Copy link
Member

Choose a reason for hiding this comment

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

Why not use Objects#hashCode() on this?

}

@Override
public String toString()
{
Expand Down Expand Up @@ -211,6 +235,30 @@ public int compareTo( Item item )
}
}

@Override
public boolean equals( Object o )
Copy link
Member

Choose a reason for hiding this comment

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

Why not use Objects#equals() on this?

{
if ( this == o )
{
return true;
}
if ( o == null || getClass() != o.getClass() )
{
return false;
}

LongItem longItem = (LongItem) o;

return value == longItem.value;

}

@Override
public int hashCode()
Copy link
Member

Choose a reason for hiding this comment

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

Why not use Objects#hashCode() on this?

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 for? It's a single object, not a list.

{
return (int) ( value ^ ( value >>> 32 ) );
}

@Override
public String toString()
{
Expand Down Expand Up @@ -272,6 +320,29 @@ public int compareTo( Item item )
}

@Override
public boolean equals( Object o )
Copy link
Member

Choose a reason for hiding this comment

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

same here...

{
if ( this == o )
{
return true;
}
if ( o == null || getClass() != o.getClass() )
{
return false;
}

BigIntegerItem that = (BigIntegerItem) o;

return value.equals( that.value );

}

@Override
public int hashCode()
{
return value.hashCode();
}

public String toString()
{
return value.toString();
Expand Down Expand Up @@ -384,6 +455,29 @@ public int compareTo( Item item )
}

@Override
public boolean equals( Object o )
Copy link
Member

Choose a reason for hiding this comment

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

and here...

{
if ( this == o )
{
return true;
}
if ( o == null || getClass() != o.getClass() )
{
return false;
}

StringItem that = (StringItem) o;

return value.equals( that.value );

}

@Override
public int hashCode()
{
return value.hashCode();
}

public String toString()
{
return value;
Expand Down Expand Up @@ -583,8 +677,6 @@ else if ( Character.isDigit( c ) )
list = (ListItem) stack.pop();
list.normalize();
}

canonical = items.toString();
}

private static Item parseItem( boolean isDigit, String buf )
Expand Down Expand Up @@ -631,19 +723,23 @@ public String toString()

public String getCanonical()
{
if ( canonical == null )
{
canonical = items.toString();
}
return canonical;
}

@Override
public boolean equals( Object o )
{
return ( o instanceof ComparableVersion ) && canonical.equals( ( (ComparableVersion) o ).canonical );
return ( o instanceof ComparableVersion ) && items.equals( ( (ComparableVersion) o ).items );
}

@Override
public int hashCode()
{
return canonical.hashCode();
return items.hashCode();
}

// CHECKSTYLE_OFF: LineLength
Expand Down
88 changes: 54 additions & 34 deletions ...n-artifact/src/main/java/org/apache/maven/artifact/versioning/DefaultArtifactVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
*/

import java.util.StringTokenizer;
import java.util.regex.Pattern;
import java.util.NoSuchElementException;

import static org.apache.commons.lang3.math.NumberUtils.isDigits;

/**
* Default implementation of artifact versioning.
Expand Down Expand Up @@ -128,30 +128,24 @@ public final void parseVersion( String version )

if ( part2 != null )
{
try
if ( part2.length() == 1 || !part2.startsWith( "0" ) )
{
if ( ( part2.length() == 1 ) || !part2.startsWith( "0" ) )
{
buildNumber = Integer.valueOf( part2 );
}
else
buildNumber = tryParseInt( part2 );
if ( buildNumber == null )
{
qualifier = part2;
}
}
catch ( NumberFormatException e )
else
{
qualifier = part2;
}
}

if ( ( !part1.contains( "." ) ) && !part1.startsWith( "0" ) )
{
try
{
majorVersion = Integer.valueOf( part1 );
}
catch ( NumberFormatException e )
majorVersion = tryParseInt( part1 );
if ( majorVersion == null )
{
// qualifier is the whole version, including "-"
qualifier = version;
Expand All @@ -163,30 +157,42 @@ public final void parseVersion( String version )
boolean fallback = false;

StringTokenizer tok = new StringTokenizer( part1, "." );
try
if ( tok.hasMoreTokens() )
{
majorVersion = getNextIntegerToken( tok );
if ( tok.hasMoreTokens() )
{
minorVersion = getNextIntegerToken( tok );
}
if ( tok.hasMoreTokens() )
if ( majorVersion == null )
{
incrementalVersion = getNextIntegerToken( tok );
fallback = true;
}
if ( tok.hasMoreTokens() )
}
else
{
fallback = true;
}
if ( tok.hasMoreTokens() )
{
minorVersion = getNextIntegerToken( tok );
if ( minorVersion == null )
{
qualifier = tok.nextToken();
fallback = Pattern.compile( "\\d+" ).matcher( qualifier ).matches();
fallback = true;
}

// string tokenizer won't detect these and ignores them
if ( part1.contains( ".." ) || part1.startsWith( "." ) || part1.endsWith( "." ) )
}
if ( tok.hasMoreTokens() )
{
incrementalVersion = getNextIntegerToken( tok );
if ( incrementalVersion == null )
{
fallback = true;
}
}
catch ( NumberFormatException e )
if ( tok.hasMoreTokens() )
{
qualifier = tok.nextToken();
fallback = isDigits( qualifier );
}

// string tokenizer won't detect these and ignores them
if ( part1.contains( ".." ) || part1.startsWith( "." ) || part1.endsWith( "." ) )
{
fallback = true;
}
Expand All @@ -204,19 +210,33 @@ public final void parseVersion( String version )
}

private static Integer getNextIntegerToken( StringTokenizer tok )
{
String s = tok.nextToken();
if ( ( s.length() > 1 ) && s.startsWith( "0" ) )
{
return null;
michael-o marked this conversation as resolved.
Show resolved Hide resolved
}
return tryParseInt( s );
}

private static Integer tryParseInt( String s )
{
try
{
String s = tok.nextToken();
if ( ( s.length() > 1 ) && s.startsWith( "0" ) )
if ( !isDigits( s ) )
{
return null;
}
long longValue = Long.parseLong( s );
if ( longValue > Integer.MAX_VALUE )
Copy link
Member

Choose a reason for hiding this comment

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

Again, change in semantics with long. Dislike on this one. Why all the hassle, why not use NumberUtils from Commons Lang 3?

Copy link
Contributor Author

@oehme oehme Apr 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Because it throws an exception internally, which is what we're trying to avoid in the first place here. We don't want to throw exceptions on common code paths.

Also, we're not changing semantics here, we are still returning an int. Going through long is only done for the very common case of big numbers that would fail with a NumberFormatException on Integer.parseInt

Copy link

Choose a reason for hiding this comment

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

+1 to @oehme, however it is probably worth adding a comment that explains this (the reason behind tryParseInt method and the reason behind parseLong).

{
throw new NumberFormatException( "Number part has a leading 0: '" + s + "'" );
return null;
}
return Integer.valueOf( s );
return (int) longValue;
}
catch ( NoSuchElementException e )
catch ( NumberFormatException e )
{
throw new NumberFormatException( "Number is invalid" );
return null;
}
}

Expand Down
10 changes: 7 additions & 3 deletions ...rc/main/java/org/apache/maven/artifact/handler/manager/DefaultArtifactHandlerManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@ public class DefaultArtifactHandlerManager
@Requirement( role = ArtifactHandler.class )
private Map<String, ArtifactHandler> artifactHandlers;

private Map<String, ArtifactHandler> unmanagedHandlers = new ConcurrentHashMap<>();
private Map<String, ArtifactHandler> allHandlers = new ConcurrentHashMap<>();

public ArtifactHandler getArtifactHandler( String type )
{
ArtifactHandler handler = unmanagedHandlers.get( type );
ArtifactHandler handler = allHandlers.get( type );

if ( handler == null )
{
Expand All @@ -53,6 +53,10 @@ public ArtifactHandler getArtifactHandler( String type )
{
handler = new DefaultArtifactHandler( type );
}
else
{
allHandlers.put( type, handler );
Copy link
Member

Choose a reason for hiding this comment

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

What side effects may this cause if we don't receive a new instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The handlers have no mutation methods, so I don't see what side effects this would cause. I'd say a stateful handler would be a terrible implementation.

}
}

return handler;
Expand All @@ -61,7 +65,7 @@ public ArtifactHandler getArtifactHandler( String type )
public void addHandlers( Map<String, ArtifactHandler> handlers )
{
// legacy support for maven-gpg-plugin:1.0
unmanagedHandlers.putAll( handlers );
allHandlers.putAll( handlers );
}

@Deprecated
Expand Down
Loading