Skip to content

Optimize the nameMap of ZipFile #378

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

Open
Glavo wants to merge 8 commits into
base: master
Choose a base branch
from
+42 −11
Open

Optimize the nameMap of ZipFile #378

Changes from 1 commit
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
57 changes: 41 additions & 16 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 @@ -371,9 +372,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.

@DamnedElric DamnedElric Nov 28, 2023

Copy link
Copy Markdown

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.

@Glavo Glavo Dec 8, 2023

Copy link
Copy Markdown
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.
Comment thread
Glavo marked this conversation as resolved.
Outdated
*/
private Map<String, List<ZipArchiveEntry>> duplicateNameMap = null;
Comment thread
Glavo marked this conversation as resolved.
Outdated

/**
* The encoding to use for file names and the file comment.
Expand Down Expand Up @@ -792,11 +798,24 @@ private BoundedArchiveInputStream createBoundedInputStream(final long start, fin

private void fillNameMap() {
entries.forEach(ze -> {
// entries is filled in populateFromCentralDirectory and
// 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);
ZipArchiveEntry firstEntry = nameMap.putIfAbsent(name, ze);
Comment thread
Glavo marked this conversation as resolved.
Outdated

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

List<ZipArchiveEntry> entriesOfThatName = duplicateNameMap.computeIfAbsent(name, k -> {
ArrayList<ZipArchiveEntry> list = new ArrayList<>(2);

@garydgregory garydgregory May 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why 2?

@Glavo Glavo May 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why 2?

This list is created when there are two entries with the same name.

If no more entries with the same name are encountered later, using 2 as the initial capacity can reduce unnecessary memory footpoint.

@Glavo Glavo May 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason for using ArrayList instead of LinkedList here is that ArrayList is almost always better than LinkedList.

In general, we can calculate the memory footprint of a lists with N elements like this:

  • LinkedList: 24 + 24*N
  • ArrayList:
    • Minimal (internal array is fully used, no wasted space):
      24 (ArrayList) + 16 (array object header) + 4*N
    • Maximum (internal array has just grown, the reserved space is the largest):
      24 (ArrayList) + 16 (array object header) + 6*N

If the list has only two elements, ArrayList with 2 as the initial capacity saves 24 bytes of memory compared to LinkedList.

As the list grows, the memory footprint of ArrayList is usually only 1/6~1/4 of that of LinkedList.

The combined memory footprint of all objects allocated during ArrayList construction (including those small temporary arrays) is smaller than the long-term memory footprint of LinkedList, so using ArrayList is more memory efficient.

For smaller lists, iterating over a LinkedList might be faster, but ArrayList has better data locality. So usually we have no reason to use LinkedList, ArrayList/ArrayDeque are better choices.

list.add(firstEntry);
return list;
});

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

Expand Down Expand Up @@ -868,9 +887,12 @@ public Enumeration<ZipArchiveEntry> getEntries() {
* @since 1.6
*/
public Iterable<ZipArchiveEntry> getEntries(final String name) {
final List<ZipArchiveEntry> entriesOfThatName = nameMap.get(name);
return entriesOfThatName != null ? entriesOfThatName
: Collections.emptyList();
List<ZipArchiveEntry> entriesOfThatName = duplicateNameMap == null ? null : duplicateNameMap.get(name);
Comment thread
Glavo marked this conversation as resolved.
Outdated
if (entriesOfThatName == null) {
ZipArchiveEntry entry = nameMap.get(name);
entriesOfThatName = entry == null ? Collections.emptyList() : Collections.singletonList(entry);
}
return entriesOfThatName;
}

/**
Expand Down Expand Up @@ -899,13 +921,17 @@ public Enumeration<ZipArchiveEntry> getEntriesInPhysicalOrder() {
* @since 1.6
*/
public Iterable<ZipArchiveEntry> getEntriesInPhysicalOrder(final String name) {
ZipArchiveEntry[] entriesOfThatName = ZipArchiveEntry.EMPTY_ARRAY;
final LinkedList<ZipArchiveEntry> linkedList = nameMap.get(name);
if (linkedList != null) {
entriesOfThatName = linkedList.toArray(entriesOfThatName);
Arrays.sort(entriesOfThatName, offsetComparator);
if (duplicateNameMap != null) {
List<ZipArchiveEntry> list = duplicateNameMap.get(name);
if (list != null) {
ZipArchiveEntry[] entriesOfThatName = list.toArray(ZipArchiveEntry.EMPTY_ARRAY);
Comment thread
Glavo marked this conversation as resolved.
Outdated
Arrays.sort(entriesOfThatName, offsetComparator);
return Arrays.asList(entriesOfThatName);
}
}
return Arrays.asList(entriesOfThatName);

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

/**
Expand All @@ -921,8 +947,7 @@ public Iterable<ZipArchiveEntry> getEntriesInPhysicalOrder(final String name) {
* {@code null} if not present.
*/
public ZipArchiveEntry getEntry(final String name) {
final LinkedList<ZipArchiveEntry> entriesOfThatName = nameMap.get(name);
return entriesOfThatName != null ? entriesOfThatName.getFirst() : null;
return nameMap.get(name);
}

/**
Expand Down