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

Optimize the nameMap of ZipFile #378

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Optimize the nameMap of ZipFile #378

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7a3e348
Optimize the nameMap of ZipFile
Glavo Apr 10, 2023
4019b26
Add final modifiers
Glavo May 9, 2023
77585b4
Add final modifiers
Glavo May 9, 2023
c07b7df
flip branches
Glavo May 9, 2023
eec6ab5
Merge branch 'master' into linked-list
Glavo Jul 18, 2023
71c3641
Delete EMPTY_LINKED_LIST
Glavo Jul 18, 2023
d0e5365
Merge remote-tracking branch 'origin/master' into linked-list
Glavo Nov 20, 2023
b7cb0f0
Update comments
Glavo Dec 8, 2023
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
2 changes: 0 additions & 2 deletions src/main/java/org/apache/commons/compress/archivers/zip/ZipArchiveEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Date;
import java.util.LinkedList;
import java.util.List;
import java.util.NoSuchElementException;
import java.util.Objects;
Expand Down Expand Up @@ -217,7 +216,6 @@ public enum NameSource {
}

static final ZipArchiveEntry[] EMPTY_ARRAY = {};
static LinkedList<ZipArchiveEntry> EMPTY_LINKED_LIST = new LinkedList<>();

public static final int PLATFORM_UNIX = 3;
public static final int PLATFORM_FAT = 0;
Expand Down
50 changes: 41 additions & 9 deletions src/main/java/org/apache/commons/compress/archivers/zip/ZipFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
Expand Down Expand Up @@ -368,9 +369,14 @@ public static void closeQuietly(final ZipFile zipFile) {
private final List<ZipArchiveEntry> entries = new LinkedList<>();

/**
* Maps String to list of ZipArchiveEntrys, name -> actual entries.
* Maps a string to the first entry named it.
Copy link

Choose a reason for hiding this comment

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

Nitpicking: "Maps a string to the first entry by that name" sounds clearer to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nitpicking: "Maps a string to the first entry by that name" sounds clearer to me.

I'm not good at English so thank you very much for pointing out the poor grammar.

*/
private final Map<String, LinkedList<ZipArchiveEntry>> nameMap = new HashMap<>(HASH_SIZE);
private final Map<String, ZipArchiveEntry> nameMap = new HashMap<>(HASH_SIZE);

/**
* If multiple entries have the same name, maps the name to entries named it.
Glavo marked this conversation as resolved.
Show resolved Hide resolved
*/
private Map<String, List<ZipArchiveEntry>> duplicateNameMap;

/**
* The encoding to use for file names and the file comment.
Expand Down Expand Up @@ -749,8 +755,21 @@ private void fillNameMap() {
// entries are filled in populateFromCentralDirectory and
// never modified
final String name = ze.getName();
final LinkedList<ZipArchiveEntry> entriesOfThatName = nameMap.computeIfAbsent(name, k -> new LinkedList<>());
entriesOfThatName.addLast(ze);
final ZipArchiveEntry firstEntry = nameMap.putIfAbsent(name, ze);

if (firstEntry != null) {
if (duplicateNameMap == null) {
duplicateNameMap = new HashMap<>();
}

final List<ZipArchiveEntry> entriesOfThatName = duplicateNameMap.computeIfAbsent(name, k -> {
final ArrayList<ZipArchiveEntry> list = new ArrayList<>(2);
Glavo marked this conversation as resolved.
Show resolved Hide resolved
list.add(firstEntry);
return list;
});

entriesOfThatName.add(ze);
}
});
}

Expand Down Expand Up @@ -820,7 +839,13 @@ public Enumeration<ZipArchiveEntry> getEntries() {
* @since 1.6
*/
public Iterable<ZipArchiveEntry> getEntries(final String name) {
return nameMap.getOrDefault(name, ZipArchiveEntry.EMPTY_LINKED_LIST);
Copy link

Choose a reason for hiding this comment

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

You mentioned a potential 30% slowdown in this method in return for additional safety, which may or may not be a fair tradeoff. But I'm a bit concerned that this change potentially breaks the current behaviour. As you say, this currently returns a modifiable LinkedList, which some people may (though I hope not) depend on? Because it requires an explicit cast, I'm not too worried.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the slowdown is only in case there are duplicit entries which is extremely unlikely. I'd still simplify the code, as according to earlier comment.

final List<ZipArchiveEntry> entriesOfThatName = duplicateNameMap == null ? null : duplicateNameMap.get(name);
if (entriesOfThatName == null) {
final ZipArchiveEntry entry = nameMap.get(name);
return entry != null ? Collections.singletonList(entry) : Collections.emptyList();
} else {
return Collections.unmodifiableList(entriesOfThatName);
}
}

/**
Expand All @@ -847,8 +872,16 @@ public Enumeration<ZipArchiveEntry> getEntriesInPhysicalOrder() {
* @since 1.6
*/
public Iterable<ZipArchiveEntry> getEntriesInPhysicalOrder(final String name) {
final LinkedList<ZipArchiveEntry> linkedList = nameMap.getOrDefault(name, ZipArchiveEntry.EMPTY_LINKED_LIST);
return Arrays.asList(sortByOffset(linkedList.toArray(ZipArchiveEntry.EMPTY_ARRAY)));
if (duplicateNameMap != null) {
final List<ZipArchiveEntry> list = duplicateNameMap.get(name);
if (list != null) {
final ZipArchiveEntry[] entriesOfThatName = list.toArray(ZipArchiveEntry.EMPTY_ARRAY);
return Arrays.asList(sortByOffset(entriesOfThatName));
}
}

final ZipArchiveEntry entry = nameMap.get(name);
return entry != null ? Collections.singletonList(entry) : Collections.emptyList();
}

/**
Expand All @@ -862,8 +895,7 @@ public Iterable<ZipArchiveEntry> getEntriesInPhysicalOrder(final String name) {
* @return the ZipArchiveEntry corresponding to the given name - or {@code null} if not present.
*/
public ZipArchiveEntry getEntry(final String name) {
final LinkedList<ZipArchiveEntry> entries = nameMap.get(name);
return entries != null ? entries.getFirst() : null;
return nameMap.get(name);
}

/**
Expand Down