Memory improvements related to PathTree and Manifests #42492
Conversation
quarkus-bot
bot
added
the
area/devtools
| private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); | ||
|
|
||
| protected OpenArchivePathTree(FileSystem fs) { | ||
| super(fs.getPath("/"), pathFilter, ArchivePathTree.this); | ||
| protected OpenArchivePathTree(Path archive, FileSystem fs) { |
There was a problem hiding this comment.
It doesn't look like archive needs to be passed in here.
There was a problem hiding this comment.
Actually it needs to be passed but you're right, I wasn't setting things properly.
There was a problem hiding this comment.
Why does it need to be passed in?
There was a problem hiding this comment.
Because of the hashCode/equals. We need something stable that is not leaking for them. The path of the archive makes perfect sense for that.
There was a problem hiding this comment.
I mean archive from the outer class would be enough for that
There was a problem hiding this comment.
I need it for SharedOpenArchivePathTree which extends this class.
...ndent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/OpenContainerPathTree.java
Show resolved
Hide resolved
gsmet
force-pushed
the
various-memory-improvements
branch
from
c222639
to
683be07
Compare
|
|
||
| @Override | ||
| public Collection<Path> getRoots() { | ||
| return List.of(getRootPath()); |
There was a problem hiding this comment.
In the current implementation, it would be getContainerPath()
There was a problem hiding this comment.
Are you sure? Because in the case of an archive, we have:
containerPath: the path to the archive itself (for instance the directory or the path to the jar) - this doesn't exist in the previous implementationrootPath: the path to the root of the archive. In the case of a directory, it's the same (the directory itself), in the case of an archive it's aZipPathto the/of the archive
For me getRoots() returned actionable roots so they should use getRootPath().
There was a problem hiding this comment.
For me getRoots() returned actionable roots so they should use getRootPath().
For a directory-backed PathTree - yes. For an ArchivePathTree it's the archive file. But I see your point if it's meant to replace the DirectoryPathTree.
There was a problem hiding this comment.
OK, I checked the callers and apparently it's used to get the path of the archive. I made the change and added a note to the javadoc.
Will push shortly.
Some manifests are quite large, for instance the one for commons-codec is 21 KB. Given most of the information in the Manifest are of no interest to us, let's trim the Manifest to only the information we need. Also we still have the problem of us keeping the jars open multiple times in test and dev mode so it amplifies this issue. In some cases, I ended up with 1700+ jars opened so the Manifests add up quite quickly.
gsmet
force-pushed
the
various-memory-improvements
branch
from
683be07
to
488cfbc
Compare
A Path can actually contain references to the filesystem: this is the case of the ZipPath. We want to make sure we don't keep ZipFileSystem around when a path tree is closed so we also need to nullify the path. Finally, we make sure only DirectoryPathTree is marked as Serializable as we only serialize the workspace directories.
gsmet
force-pushed
the
various-memory-improvements
branch
from
488cfbc
to
6780c2c
Compare
Note
This PR is part of a coordinated effort to address the OOM reported in #42471:
See each commit message for the details.
Related to #42471