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

Gzip artifacts #4972

Merged
merged 1 commit into from
Jul 7, 2020
Merged

Gzip artifacts #4972

merged 1 commit into from
Jul 7, 2020

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Jun 21, 2020

Here is the test release

Change in size: ~ 25 MB -> ~ 8 MB (gzipped)

The time to gzip during the dist build takes a somewhat considerable amount of time tho.
Having already compiled artifacts this takes in debug mode:

~/dev/rust-analyzer (feat/gzip-binaries) $ time cargo xtask dist
    Finished dev [unoptimized] target(s) in 0.06s
     Running `target/debug/xtask dist`
> cargo build --manifest-path ./crates/rust-analyzer/Cargo.toml --bin rust-analyzer --release
    Finished release [optimized] target(s) in 0.05s
> strip ./target/release/rust-analyzer

real    0m34.331s
user    0m34.245s
sys     0m0.078s

In release mode this is much faster:

~/dev/rust-analyzer (feat/gzip-binaries) $ time cargo run -p xtask --release -- dist
    Finished release [optimized] target(s) in 0.04s
     Running `target/release/xtask dist`
> cargo build --manifest-path ./crates/rust-analyzer/Cargo.toml --bin rust-analyzer --release
    Finished release [optimized] target(s) in 0.06s
> strip ./target/release/rust-analyzer

real    0m2.401s

[UPD] adding a profile override for miniz_oxide does the thing to ensure good performrance

We might need to notify all other ra plugins' maintainers about the change in our GH releases if we merge this PR, or we could leave uncompressed files along with gzipped for a while until everyone migrates.

xtask/src/dist.rs Outdated Show resolved Hide resolved
@Veetaha Veetaha marked this pull request as draft June 21, 2020 16:24
editors/code/src/main.ts Outdated Show resolved Hide resolved
@Veetaha
Copy link
Contributor Author

Veetaha commented Jun 21, 2020

Hmm, actually I think we might need to compile xtask in release mode.
Because when running cargo xtask dist in release mode it takes 10 times fewer time.

@lnicola
Copy link
Member

lnicola commented Jun 21, 2020

I guess we could use a profile override to compile flate2 with optimizations.

@Veetaha
Copy link
Contributor Author

Veetaha commented Jun 21, 2020

Hmm, that's a nice idea

@Veetaha Veetaha marked this pull request as ready for review June 21, 2020 17:00
@Veetaha Veetaha force-pushed the feat/gzip-binaries branch from e2eb661 to 70da811 Compare June 21, 2020 17:02
@Veetaha
Copy link
Contributor Author

Veetaha commented Jun 21, 2020

Squashed the changes

@Veetaha Veetaha force-pushed the feat/gzip-binaries branch from 70da811 to e223830 Compare June 21, 2020 17:08
@killercup
Copy link
Member

FTR, I was about to say that github most likely already uses deflate as HTTP encoding -- but then I checked and according to curl --compressed -L https://github.com/rust-analyzer/rust-analyzer/releases/download/nightly/rust-analyzer-mac -w '%{size_download}' -so /dev/null it doesn't.

@Veetaha Veetaha marked this pull request as draft June 22, 2020 17:11
@matklad
Copy link
Member

matklad commented Jun 23, 2020

Not sure if we need to do this -- I'd rather keep this minimal. The end-goal is distribution via rustup, so additional complexity here won't be worthwhile over the long run. This will also make life harder for folks who maintain other editor plugins.

@matklad
Copy link
Member

matklad commented Jun 23, 2020

What would help long term is pushing this PR over the finish line: rust-lang/rust#72978 (it currently misses the bit about vendoring).

@lnicola
Copy link
Member

lnicola commented Jun 23, 2020

@matklad do you plan to keep offering binary releases after rust-lang/rust#72978?

@matklad
Copy link
Member

matklad commented Jun 23, 2020

@lnicola for at least medium term -- yes

@matklad
Copy link
Member

matklad commented Jul 1, 2020

Hm, I am thinking that maybe we should enable debug = 1 for release profile? Lack of line numbers from reports is frustraiting. I guess if we do that, it might make sense to enable compression

@lnicola
Copy link
Member

lnicola commented Jul 1, 2020

Do we even get backtraces? I think we strip the binaries.

@lnicola
Copy link
Member

lnicola commented Jul 1, 2020

Maybe we should enable debug info for the binary releases, but not for source builds.

@Veetaha
Copy link
Contributor Author

Veetaha commented Jul 1, 2020

@matklad this is the reason we don't strip binaries for nightlies, haven't found a PR, but here is the commit: ffb7ea6

@matklad
Copy link
Member

matklad commented Jul 1, 2020 via email

@Veetaha Veetaha force-pushed the feat/gzip-binaries branch 5 times, most recently from 5697716 to 97f6f8c Compare July 1, 2020 18:41
@Veetaha Veetaha marked this pull request as ready for review July 1, 2020 19:05
@Veetaha
Copy link
Contributor Author

Veetaha commented Jul 1, 2020

Removed stripping.
Also, I am leaving bare binaries output from cargo xtask dist because removing non-compressed binaries from Github releases is a breaking change for other plugins that download our releases (coc.nvim rust-analyzer).
You can see the updated example release in my fork repo: https://github.com/Veetaha/rust-analyzer/releases/tag/2020-07-01
This is strange that linux binaries are 40MB unzipped, but windows/mac ones are only 32/33MB...
cc @matklad @lnicola

run!(
"cargo build --manifest-path ./crates/rust-analyzer/Cargo.toml --bin rust-analyzer --release"
// We'd want to add, but that requires setting the right linker somehow
// --features=jemalloc
)?;
if !nightly {
run!("strip ./target/release/rust-analyzer")?;
}
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@Veetaha
Copy link
Contributor Author

Veetaha commented Jul 1, 2020

I also wonder whether we really need to have a special case .exe extension on windows?

@Veetaha
Copy link
Contributor Author

Veetaha commented Jul 1, 2020

Tested this on host Ubuntu and VirtualBox windows, both work.

@bjorn3
Copy link
Member

bjorn3 commented Jul 1, 2020

$ size -A -x rust-analyzer-linux
section                   size        addr
.interp                   0x1c       0x270
.note.ABI-tag             0x20       0x28c
.note.gnu.build-id        0x24       0x2ac
.gnu.hash               0x523c       0x2d0
.dynsym                0x13c68      0x5510
.dynstr                0x4b686     0x19178
.gnu.version            0x1a5e     0x647fe
.gnu.version_r           0x190     0x66260
.rela.dyn              0xe1900     0x663f0
.init                     0x1a    0x147cf0
.plt                      0x10    0x147d10
.plt.got                  0xf0    0x147d20
.text                0x16dc110    0x147e10
.fini                      0x9   0x1823f20
.rodata                0xe5de3   0x1823f40
.eh_frame_hdr          0x6f574   0x1909d24
.eh_frame             0x33ca64   0x1979298
.gcc_except_table     0x17f8dc   0x1cb5cfc
.tdata                    0x60   0x2035700
.tbss                    0x458   0x2035760
.init_array               0x10   0x2035760
.fini_array                0x8   0x2035770
.jcr                       0x8   0x2035778
.data.rel.ro           0x98c10   0x2035780
.dynamic                 0x220   0x20ce390
.got                    0x6a38   0x20ce5b0
.data                    0x209   0x20d5000
.bss                     0xf08   0x20d5210
.comment                  0x7f         0x0
.debug_aranges          0x8860         0x0
.debug_pubnames        0x31761         0x0
.debug_info            0xa1f13         0x0
.debug_abbrev           0x197e         0x0
.debug_line            0x5ee2f         0x0
.debug_frame             0xf18         0x0
.debug_str             0xa9bdf         0x0
.debug_loc             0x27d48         0x0
.debug_macinfo            0x19         0x0
.debug_pubtypes          0x132         0x0
.debug_ranges          0x6ea70         0x0
Total                0x2153c8d

$ size -A -x ~/Downloads/rust-analyzer-linux | grep debug | tr -s ' ' | cut -d' ' -f2 | tr '\n' '+'
0x8860+0x31761+0xa1f13+0x197e+0x5ee2f+0xf18+0xa9bdf+0x27d48+0x19+0x132+0x6ea70+
$ echo $((0x8860+0x31761+0xa1f13+0x197e+0x5ee2f+0xf18+0xa9bdf+0x27d48+0x19+0x132+0x6ea70)) # debuginfo (sum(.debug_*))
2611835
# = 0x27da7b = ~2.5MiB

$ echo $((0x6f574+0x33ca64+0x17f8dc)) # unwind info (.eh_frame_hdr + .eh_frame + .gcc_except_table)
5421236
# 0 0x52b8b4 = ~5.2MiB

$ echo $((0x2153c8d-0x16dc110-0xe5de3)) # total - .text - .rodata
10034586
# = 0x991d9a = ~9.6MiB

Most of the non code/data part seems to be debuginfo and unwind info.

@Veetaha
Copy link
Contributor Author

Veetaha commented Jul 1, 2020

@bjorn3 is it possible to get these stats for windows/mac?

@bjorn3
Copy link
Member

bjorn3 commented Jul 1, 2020

For MachO files objdump --headers should work I think. It has a Size column for every section. If you add | grep "\." then flags are filtered out, reducing visual clutter of the output.

@lnicola
Copy link
Member

lnicola commented Jul 3, 2020

$ llvm-objdump --headers rust-analyzer-windows.exe                                                                                                                                                   

rust-analyzer-windows.exe:	file format COFF-x86-64

Sections:
Idx Name          Size     VMA              Type
  0 .text         019b581c 0000000140001000 TEXT
  1 .rdata        007129f6 00000001419b7000 DATA
  2 .data         00000c00 00000001420ca000 DATA
  3 .pdata        0019a0dc 00000001420cc000 DATA
  4 .reloc        00011b68 0000000142267000 DATA

@matklad
Copy link
Member

matklad commented Jul 7, 2020

bors d+

(some conflicts in cargo.lock)

(miniz_oxide will be deduped after rust-lang/backtrace-rs#354)

@bors
Copy link
Contributor

bors bot commented Jul 7, 2020

✌️ Veetaha can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@matklad
Copy link
Member

matklad commented Jul 7, 2020

#5250 updates miniz

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

Override miniz_oxide to build it with optimizations

Building this crate with optimizations decreases the gzipping
part of `cargo xtask dist` from `30-40s` down to `3s`,
the overhead for `rustc` to apply optimizations is miserable on this background
@Veetaha Veetaha force-pushed the feat/gzip-binaries branch from 97f6f8c to f92bfb5 Compare July 7, 2020 20:30
@Veetaha
Copy link
Contributor Author

Veetaha commented Jul 7, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 7, 2020

@bors bors bot merged commit 56ade20 into rust-lang:master Jul 7, 2020
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.

5 participants