Shallow clone trees to save on memory usage #268
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alex Goodman <[email protected]>
Benchmark Test ResultsBenchmark results from the latest changes vs base branch |
Signed-off-by: Alex Goodman <[email protected]>
kzantow
previously approved these changes
Oct 14, 2024
|
After talking with @willmurphyscode I'm going to hold off on merging in order to demonstrate a little more proof of this working as intended in syft. If all goes well I think this will land on the order of days 🎉 |
|
The test we still want to do is get a big image that has different file types for the same location, and edit the file, and prove that the filetree object doesn't get mutated. This can probably be done with the stereoscope example application. Basically, make an image that, for a given path, has layers where that path is a directory with children, then where it's a file, where it's a link, where it's a file with different contents, etc. Basically fuzzing the tree to prove that the structs that are now being shallow-copied won't be mutated. |
willmurphyscode
dismissed
kzantow’s stale review
we decided to do additional testing before merging
|
I ran a test, and I think we have some differences here. Overview
Setup❯ tree .
.
├── Dockerfile
└── chimera
├── a.txt
└── b.txtFROM debian:trixie-slim
# makes /chimera/a.txt /chimera/b.txt
COPY . /
# make a layer where the dir is empty
RUN rm /chimera/a.txt /chimera/b.txt
RUN rmdir /chimera
RUN touch /chimera
RUN rm /chimera
RUN ln -s /etc/hostname /chimera
RUN unlink /chimera
RUN mkdir /chimeraSaving results like go run ./examples/basic.go localhost/aggressive > willtmp/before.txt
git checkout shallow-tree-clone
git rebase origin/main
go run ./examples/basic.go localhost/aggressive > willtmp/after-with-rebase.txtResultsRunning --- willtmp/before.txt 2024-11-23 07:39:21
+++ willtmp/after-with-rebase.txt 2024-11-23 07:44:20
@@ -10336,8 +10336,6 @@
/bin/znew
/boot
/chimera
- /chimera/a.txt
- /chimera/b.txt
/dev
/etc
/etc/.pwd.lockI think some more investigation is needed before we merge this. A reasonable next step is probably to write a failing test that exhibits this bug and push it to this branch. The above testing proves there is a difference, but not much about what the cause is. |
Signed-off-by: Will Murphy <[email protected]>
|
I've pushed a test case that passes on |
|
Thanks for taking the due diligence here and proving that this doesn't quite work in the context of syft -- I'm going to close this for now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based off of the inuse memory in stereoscope we spend of lot of memory on copying the filetree for squashing. An observation here is that all tree mutation operations always create or replace nodes, but never mutate existing nodes. This means that we can share references across the multiple trees created for the squashfs functionality for each layer.
Here is a snapshot of inuse memory today for image read for a sample image:
Here is a snapshot of inuse memory after using
map.Cloneto shallow clone existing trees:Partially fixes #233 in terms of inuse memory