-
Notifications
You must be signed in to change notification settings - Fork 30
Filesystem optimizations #67
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.
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
base: main
Are you sure you want to change the base?
Filesystem optimizations #67
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performed full review of 330685e...c02e552
Tip
⚡ Quick Actions
This review was generated by Mesa.
Actions:
Slash Commands:
/review
- Request a full code review/review latest
- Review only changes since the last review/describe
- Generate PR description. This will update the PR body or issue comment depending on your configuration/help
- Get help with Mesa commands and configuration options
2 files reviewed | 3 comments | Review on Mesa | Edit Reviewer Settings
docker cp cnt-"$app_name":/ ./.rootfs | ||
rm -f initrd || true | ||
sudo mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs | ||
sudo mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -x 0
parameter being added here should be documented or explained. The -x
flag in mkfs.erofs typically controls xattr features, and setting it to 0 disables extended attribute support. This could impact applications that rely on extended attributes for security contexts, capabilities, or other metadata. Consider documenting why this is being disabled and whether it's intentional for the optimization goals.
Agent: 🤖 General
docker cp cnt-"$app_name":/ ./.rootfs | ||
rm -f initrd || true | ||
mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs | ||
mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistency detected: This command doesn't use sudo
while the equivalent command in images/chromium-headful/build-unikernel.sh
does use sudo
. Both commands create the same initrd
file and use identical parameters. This inconsistency could lead to permission issues or different behavior between headful and headless builds. Consider standardizing the approach - either both should use sudo or neither should, depending on the execution environment and file permissions needed.
Agent: 🤖 General
docker cp cnt-"$app_name":/ ./.rootfs | ||
rm -f initrd || true | ||
mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs | ||
mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compression block size -C 131072
(128KB) is quite large and may not be optimal for all use cases. While this can improve compression ratios for large files, it can hurt performance for small file access patterns common in web applications. The PR mentions this is experimental and needs further testing - consider adding a comment explaining the rationale for this specific block size choice and making it configurable via an environment variable for easier testing of different values.
Agent: 🤖 General
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performed full review of 330685e...c02e552
Tip
⚡ Quick Actions
This review was generated by Mesa.
Actions:
Slash Commands:
/review
- Request a full code review/review latest
- Review only changes since the last review/describe
- Generate PR description. This will update the PR body or issue comment depending on your configuration/help
- Get help with Mesa commands and configuration options
2 files reviewed | 3 comments | Review on Mesa | Edit Reviewer Settings
docker cp cnt-"$app_name":/ ./.rootfs | ||
rm -f initrd || true | ||
sudo mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs | ||
sudo mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compression block size of 131072 (128KB) is quite large and may increase memory usage during filesystem operations. Consider documenting the minimum EROFS-utils version required (1.6+) for the -C and -z flags, or add version validation to prevent build failures on older systems.
Agent: 🤖 General
docker cp cnt-"$app_name":/ ./.rootfs | ||
rm -f initrd || true | ||
mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs | ||
mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker cp cnt-"$app_name":/ ./.rootfs | ||
rm -f initrd || true | ||
mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs | ||
mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performed full review of 330685e...c02e552
Tip
⚡ Quick Actions
This review was generated by Mesa.
Actions:
Slash Commands:
/review
- Request a full code review/review latest
- Review only changes since the last review/describe
- Generate PR description. This will update the PR body or issue comment depending on your configuration/help
- Get help with Mesa commands and configuration options
2 files reviewed | 3 comments | Review on Mesa | Edit Reviewer Settings
docker cp cnt-"$app_name":/ ./.rootfs | ||
rm -f initrd || true | ||
sudo mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs | ||
sudo mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter ordering could be improved for readability. Consider grouping related parameters together, e.g., placing compression-related options (-C, -z) adjacent to each other: sudo mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -z lz4 -x 0 initrd ./.rootfs
Agent: 🤖 General
docker cp cnt-"$app_name":/ ./.rootfs | ||
rm -f initrd || true | ||
mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs | ||
mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling for the mkfs.erofs command. Since this is a critical step in the build process and the new parameters could potentially fail with older EROFS versions, adding || { echo "EROFS filesystem creation failed"; exit 1; }
would provide better error reporting.
Agent: 🤖 General
docker cp cnt-"$app_name":/ ./.rootfs | ||
rm -f initrd || true | ||
mkfs.erofs --all-root -d2 -E noinline_data -b 4096 initrd ./.rootfs | ||
mkfs.erofs --all-root -d2 -E noinline_data,fragments -b 4096 -C 131072 -x 0 -z lz4 initrd ./.rootfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistency: The headful version uses sudo
for mkfs.erofs while this headless version doesn't. This could lead to different permission behaviors or failures depending on the execution environment. Consider either adding sudo
here or documenting why it's not needed for the headless build.
Agent: 🤖 General
This PR adds filesystem optimizations which on the surface exchange an ~11% response time, ~20% Chromium startup time, and ~67% filesystem build time for an image size reduction of ~48% and initial boot time reduction of ~39%. Although this PR does not depend on another PR, when combined with the other pending optimizations the benefits compound.
The advantages continue with effects through the chain. The image size reduction is generally interesting and brings additional improvements to deployment times (e.g. publish, propagation, build). Initial boot time improvements are present but might yield the most benefit outside general use cases. There are pulls on time to first response an order of magnitude larger than the boot time reduction herein.
Further testing is needed. This PR remains separate from other optimizations not only from the focus of the content and potential for unsuitability but also due to the higher potential for negative impact. If inapplicable for the default operating case it could remain an interesting option for special cases.
Some future benefits might be inherited automatically or made available as EROFS is further developed. The integration of zstandard compression is one such item that stands out.
Anecdotal testing, suggested to use EROFS 1.7:
TL;DR
Switched the unikernel filesystem to EROFS with LZ4 compression, trading a minor runtime performance cost for a ~48% reduction in image size and ~39% faster initial boot times.
Why we made these changes
To significantly reduce the final image size. Smaller images lead to faster deployment times (publishing, propagation, builds) and lower storage costs. While this optimization introduces a slight increase in response times and build times, the substantial benefits to the deployment lifecycle and boot performance make it a worthwhile trade-off to explore.
What changed?
build-unikernel.sh
scripts for bothchromium-headful
andchromium-headless
.mkfs.erofs
command is now used to build theinitrd
filesystem with the following key changes:-z lz4
).-E fragments
).131072
bytes.-x 0
).Validation
Anecdotal testing shows:
Further testing is needed, as these changes have a higher potential for negative performance impact in the default operating case.
Description generated by Mesa. Update settings