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

Use deno 1.43: binaries ~25% smaller, ~5% faster #978

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Feb 22, 2024

Note: this description is outdated.

This PR builds on #959, and bumps deno to 1.41.

With this change, the pkgx binaries size were decrease by 40MB in Linux AMD64:

image

PS: you can close this PR. I just wanted to demonstrate the benefits of upgrading to deno 1.41.

You guys know best what to do. Probably some changes in libpkgx are needed as well.

Closes #959

@mxcl
Copy link
Member

mxcl commented Feb 22, 2024

Yeah totes want it. We’re having trouble building 1.41 over at the pantry, and couldn't bump to 1.40 due to deprecated API with no replacements at that time. Once 1.41 builds we can figure it all out.

@felipecrs
Copy link
Contributor Author

Awesome!

@felipecrs felipecrs closed this Feb 22, 2024
@mxcl
Copy link
Member

mxcl commented Feb 22, 2024

FYI if you strip deno first it results in smaller binaries too.

@felipecrs
Copy link
Contributor Author

Right, which can be done even with deno 1.39. But I don't think is so urgent. :)

@jhheider
Copy link
Contributor

1.41 likely in an hour or so. or two. it's not exactly fast to build.

@felipecrs felipecrs reopened this Feb 23, 2024
@felipecrs felipecrs marked this pull request as ready for review February 23, 2024 02:41
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Feb 23, 2024
@jhheider
Copy link
Contributor

1.41.0 not ready for linux/aarch64. I pushed fixes for the other 3, but it uses their build of denort, which uses ubuntu22's glibc. You can see the outstanding issue as a comment in the package.yml.

@felipecrs
Copy link
Contributor Author

Right, and apparently, they don't allow to specify your own denort binary as well. Thanks for following it up.

@felipecrs felipecrs marked this pull request as draft February 23, 2024 02:50
@mxcl
Copy link
Member

mxcl commented Feb 23, 2024

So main remaining issue is libpkgx uses Deno.run which is deprecated but is necessary to build libpkgx with dnt so we can publish to npm because Deno.Command is not available to the dnt shims.

And deno does not make it possible to silence the shouted warning in compiled binaries about the deprecated API usage.

At least that was true with deno 1.40

@felipecrs
Copy link
Contributor Author

And deno does not make it possible to silence the shouted warning in compiled binaries about the deprecated API usage.

The binary I compiled is not throwing any warnings when invoked. Is there some specific command to trigger it?

Code_SbayEe6Xw8.mp4

@felipecrs
Copy link
Contributor Author

felipecrs commented Mar 8, 2024

@mxcl I made some additional tries and I cannot trigger any deprecation warning. Maybe this is good to go?

but it uses their build of denort, which uses ubuntu22's glibc

@jhheider, is this a no-go?

@jhheider
Copy link
Contributor

jhheider commented Mar 8, 2024

it is for building the same deno across all environments for the time being. per their notes in the PR:

    # https://github.com/denoland/deno/pull/22298
    # deno.land 1.41.0 will currently _not_ run deno compile on linux/aarch64
    # for their first official release, they're using ubuntu 22.04, which means
    # a newer glibc. Patching via the https://github.com/LukeChannings/deno-arm64
    # repo may be possible, but lets not delay the three usable arches for the rare
    # one. Revist this.

the problem is, deno downloads denort, rather than using the local one; so we'd need to hack their code to make it first check for a local denort. that's potentially possible, but i haven't dug into it. i assume our compiled denort will still work, so fixing that could be the easiest path forward.

otherwise, we could put in build logic that uses newer deno for 3/4 of our platforms, as long as it's drop-in and isn't diverging from the old one too rapidly. that'd reduce binary sizes for our most-used platforms.

@felipecrs
Copy link
Contributor Author

felipecrs commented Mar 8, 2024

Ok. I see now. Here's a quick reproduction of the problem btw (I used it to verify whether 1.41.2 had solved it or not -- it didn't).

FROM denoland/deno:1.41.2 AS build

RUN apt-get update && apt-get install unzip -y

RUN deno compile https://docs.deno.com/examples/welcome.ts

FROM ubuntu:18.04

COPY --from=build /welcome /

ADD "https://www.random.org/cgi-bin/randbyte?nbytes=10&format=h" skipcache
RUN /welcome
# install arm64 emulator
$ docker run --privileged --rm --pull=always tonistiigi/binfmt --install arm64

$ docker build . --platform=amd64
[...]
Welcome to Deno!

$ docker build . --platform=arm64
[...]
/welcome: /lib/aarch64-linux-gnu/libm.so.6: version `GLIBC_2.29' not found (required by /welcome)

I'll let you know if I find out something.

@felipecrs
Copy link
Contributor Author

felipecrs commented Mar 8, 2024

i assume our compiled denort will still work, so fixing that could be the easiest path forward.

Which "our" compiled denort? It's not in the pantry from what I can tell.

That's the easiest path forward I can think of as well. And I don't think it should be considered a workaround either, as I think it would be desirable to have control over the libs used to compile denort and therefore pkgx itself.

I think what it would take is:

  1. pkg denort in pantry
  2. apply a patch to pkgx's deno, it should download denort from the pantry rather than downloading it from the official deno binaries

The best possible solution IMO would be if deno could self-package the denort in runtime (after all it's a sub-set of the full deno binary).

@jhheider
Copy link
Contributor

jhheider commented Mar 8, 2024

current builds of deno.land also produce bin/denort (via cargo install). So, if we could first check the path, or alongside the deno binary, we'd be in good shape.

@felipecrs
Copy link
Contributor Author

Understood! Will look into it. Thanks.

@felipecrs
Copy link
Contributor Author

Checking the denort binary beside deno may not be the best option because deno is capable of cross compiling it too (by downloading the extra denort binaries).

@felipecrs
Copy link
Contributor Author

Checking the denort binary beside deno may not be the best option because deno is capable of cross compiling it too (by downloading the extra denort binaries).

But it's ok in pkgx's case because cross compilation is not used.

@jhheider take a look at my latest commit, I think it's good enough now. :D

f358c14

https://github.com/denoland/deno/blob/5d671e079a3aada2fd1efccd82a3fe4fd33c512e/cli/standalone/binary.rs#L484

The bad news however is:

ls -lah pkgx*
-rwxrwxrwx 1 felipecrs felipecrs  88M Mar  8 14:45 pkgx.deno-denort
-rwxrwxrwx 1 felipecrs felipecrs 106M Mar  8 14:44 pkgx.pkgx-denort

But it's an improvement anyway over the previous 118MB. We can probably trim this size down by compiling denort in the pipeline with some different flags. We can potentially make the final denort size even smaller than deno's own denort.

@felipecrs felipecrs marked this pull request as ready for review March 8, 2024 17:52
@jhheider
Copy link
Contributor

jhheider commented Mar 8, 2024

Good find! @mxcl what do you think? I'm hesitant to runtime.env this, since it'll break cross compiling, but this should fix our builds.

@felipecrs
Copy link
Contributor Author

Bumping Deno to 1.42, the compiled binary is now 100MB, compared to 106MB from Deno 1.41 and 118MB from Deno 1.39.

Deno 1.42 also claims to improve startup time, which should greatly help pkgx. I'm yet to benchmark it though.

@felipecrs
Copy link
Contributor Author

I did a quick and dirty benchmark file which I uploaded to the repo. Here are the results:

pkgx deno@1.40 task benchmark && pkgx deno@1.41 task benchmark && pkgx deno@1.42 task benchmark
Task benchmark deno task compile &>/dev/null && deno bench --allow-run --allow-env --seed=250 benchmarks
Check file:///home/felipecrs/repos/pkgx/benchmarks/startup_bench.ts
cpu: AMD Ryzen 7 5800X 8-Core Processor
runtime: deno 1.40.5 (x86_64-unknown-linux-gnu)

file:///home/felipecrs/repos/pkgx/benchmarks/startup_bench.ts
benchmark               time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------------ -----------------------------

group startup
git --version            4.32 ms/iter         231.4     (3.98 ms … 4.92 ms) 4.42 ms 4.83 ms 4.92 ms
pkgx git --version     528.89 ms/iter           1.9 (517.75 ms … 539.56 ms) 537.1 ms 539.56 ms 539.56 ms

summary
  git --version
   122.4x faster than pkgx git --version

Task benchmark deno task compile &>/dev/null && deno bench --allow-run --allow-env --seed=250 benchmarks
Check file:///home/felipecrs/repos/pkgx/benchmarks/startup_bench.ts
cpu: AMD Ryzen 7 5800X 8-Core Processor
runtime: deno 1.41.3 (x86_64-unknown-linux-gnu)

file:///home/felipecrs/repos/pkgx/benchmarks/startup_bench.ts
benchmark               time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------------ -----------------------------

group startup
git --version            4.39 ms/iter         227.8     (4.04 ms … 5.03 ms) 4.5 ms 4.97 ms 5.03 ms
pkgx git --version     525.04 ms/iter           1.9  (503.98 ms … 573.2 ms) 529.41 ms 573.2 ms 573.2 ms

summary
  git --version
   119.61x faster than pkgx git --version

Task benchmark deno task compile &>/dev/null && deno bench --allow-run --allow-env --seed=250 benchmarks
Check file:///home/felipecrs/repos/pkgx/benchmarks/startup_bench.ts
cpu: AMD Ryzen 7 5800X 8-Core Processor
runtime: deno 1.42.0 (x86_64-unknown-linux-gnu)

file:///home/felipecrs/repos/pkgx/benchmarks/startup_bench.ts
benchmark               time (avg)        iter/s             (min … max)       p75       p99      p995
------------------------------------------------------------------------ -----------------------------

group startup
git --version            4.32 ms/iter         231.7     (4.01 ms … 5.16 ms) 4.37 ms 5.06 ms 5.16 ms
pkgx git --version      499.3 ms/iter           2.0 (490.89 ms … 516.15 ms) 502.55 ms 516.15 ms 516.15 ms

summary
  git --version
   115.67x faster than pkgx git --version

However, I ran it several times. The results aren't consistent at all. Maybe deno bench isn't proper for such kind of testing.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Mar 29, 2024
@felipecrs
Copy link
Contributor Author

Ok, I made some changes and now the results are a bit more consistent:

scripts/benchmark.sh
Compiling with deno@1.40...
Benchmarking...
   113.24x faster than pkgx git --version
   114.81x faster than pkgx git --version
   115.91x faster than pkgx git --version
   114.68x faster than pkgx git --version
   115.35x faster than pkgx git --version
   114.02x faster than pkgx git --version
   114.56x faster than pkgx git --version
   114.52x faster than pkgx git --version
   115.32x faster than pkgx git --version
   115.06x faster than pkgx git --version
Compiling with deno@1.41...
Benchmarking...
   114.02x faster than pkgx git --version
   112.7x faster than pkgx git --version
   116.1x faster than pkgx git --version
   111.63x faster than pkgx git --version
   112.52x faster than pkgx git --version
   111.36x faster than pkgx git --version
   112.66x faster than pkgx git --version
   111.69x faster than pkgx git --version
   113.51x faster than pkgx git --version
   113.86x faster than pkgx git --version
Compiling with deno@1.42...
Benchmarking...
   111.43x faster than pkgx git --version
   110.5x faster than pkgx git --version
   113.28x faster than pkgx git --version
   111.98x faster than pkgx git --version
   111.71x faster than pkgx git --version
   113.25x faster than pkgx git --version
   124.55x faster than pkgx git --version
   112.06x faster than pkgx git --version
   113.39x faster than pkgx git --version
   112.52x faster than pkgx git --version

The lower the better.

I can see a clear improvement of 1.41 over 1.40, but 1.42 over 1.41 didn't produce any meaningful changes. The highest is higher than 1.41, but the lowest is also lower than 1.41.

@felipecrs felipecrs changed the title Use deno 1.41, make binaries ~20MB smaller Use deno 1.42, make binaries ~20MB smaller Mar 29, 2024
@felipecrs
Copy link
Contributor Author

When bk build deno.land, I noticed this:

warning: deno@1.41.3: Compiling with all symbols exported, this will result in a larger binary. Please use glibc 2.35 or later for an optimised build.

Probably this is a hint on how we can make the binaries smaller.

@felipecrs
Copy link
Contributor Author

When bk build deno.land, I noticed this:

warning: deno@1.41.3: Compiling with all symbols exported, this will result in a larger binary. Please use glibc 2.35 or later for an optimised build.

Probably this is a hint on how we can make the binaries smaller.

Ah ok, now I realize. Compiling with glibc 2.35+ would mean the compiled binaries would only work in newer systems. This in turn would also restrict pkgx itself to only run in newer systems. That's obviously undesired.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 11, 2024
@felipecrs
Copy link
Contributor Author

I would say this PR is ready to be merged (but I'd recommend merging #959 first so that this one becomes lighter).

Here are up to date stats:

image

image

I.e:

  • The binary is 25MB smaller
  • It's ~4-5% faster.

@mxcl mxcl force-pushed the main branch 2 times, most recently from 9038067 to 342373c Compare April 23, 2024 21:22
@felipecrs felipecrs changed the title Use deno 1.42, make binaries ~20MB smaller Use deno 1.43: binaries ~25% smaller, ~5% faster May 1, 2024
@felipecrs
Copy link
Contributor Author

Deno 1.43.0 has some small improvements regarding startup time:

https://deno.com/blog/v1.43#faster-es-and-commonjs-module-loading

image

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants