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.

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

perf(build): Reduce gzip compression level in packaging build context #2400

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

Conversation

tanmoysrt
Copy link
Member

@tanmoysrt tanmoysrt commented Jan 1, 2025

By default, tarfile library in python do gzip compression at level 9 (ref).

tar or gzip cli use compress level 6 by default (ref).

Anything above 5 or 6 compression level can take significant resource and time.


Locally, I have cloned 10 frappe apps (430MB) to compress with different gzip level using tarfile.

  • Result of Gzip compression level 9
     real	0m13.507s
     user	0m13.171s
     sys	0m0.327s
    
    Compressed File Size - 235MB
  • Result of Gzip compression level 5
     real	0m8.599s
     user	0m8.275s
     sys	0m0.324s
    
    Compressed File Size - 239MB

Other benchmarks - https://stephane.lesimple.fr/blog/lzop-vs-compress-vs-gzip-vs-bzip2-vs-lzma-vs-lzma2xz-benchmark-reloaded/

- By default, tarfile library do gzip compression at 9 level
- Anything above 5 or 6 can take significant resource and time
- Based on tests, result of compression of 10 frappe apps around 430MB
  - With compression level 5, it takes ~8.5s and compressed file size 239MB
  - With compress level 9, it takes ~13.5s and compressed file size 235MB

So, we are not getting too much benefit from using compression level 9
Usually in macos, we have different uid/gid than any linux system
So, in development, it was causing issue.
That's why the permission modification was added
  - frappe#1925

But, in prod it's not required.
So, we can just modify permission, when developer mode is enabled
@tanmoysrt tanmoysrt requested a review from 18alantom as a code owner January 1, 2025 11:59
Copy link

codecov bot commented Jan 1, 2025

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 38.33%. Comparing base (5e905e7) to head (c37d418).

Files with missing lines Patch % Lines
...press/doctype/deploy_candidate/deploy_candidate.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2400      +/-   ##
==========================================
- Coverage   38.33%   38.33%   -0.01%     
==========================================
  Files         378      378              
  Lines       29534    29536       +2     
==========================================
  Hits        11322    11322              
- Misses      18212    18214       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ankush
Copy link
Member

ankush commented Jan 6, 2025

@tanmoysrt you might wanna add all existing violations to cspell whitelist in one shot, else it will become a continuous annoyance and won't help much.

@tanmoysrt
Copy link
Member Author

@tanmoysrt you might wanna add all existing violations to cspell whitelist in one shot, else it will become a continuous annoyance and won't help much.

Added those words in whitelist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants