feat(cli): check for updates in background

[piscisaureus]

No description provided.

[littledivy]

This needs to be done in a way that doesn't impact startup time. It's too much overhead:

[piscisaureus]

This needs to be done in a way that doesn't impact startup time. It's too much overhead:

Sure. How?

[bartlomieju]

This needs to be done in a way that doesn't impact startup time. It's too much overhead:

Sure. How?

I think that check_for_upgrades() routine should be async and be called with tokio::spawn() using tokio::select!() with get_subcommand(flags).

[axetroy]

This may not only increase startup time, but also cause unexpected network access. In some cases, security warnings may be triggered. (such as internal networks without Internet at all)

It is also a question whether Deno's cloud service will access the network every time when it is started.

[ry]

What you can do is write an empty file $DENO_DIR/last_update_check. We can use the mtime of that file to log last update, allowing us to get away with just a single extra stat syscall.

I think that check_for_upgrades() routine should be async and be called with tokio::spawn() using tokio::select!() with get_subcommand(flags).

That won't work. We want to present "There's a new version 1.65.2" before anything else gets printed to stdout. Otherwise it will be quite confusing.

[bartlomieju]

I think that check_for_upgrades() routine should be async and be called with tokio::spawn() using tokio::select!() with get_subcommand(flags).

That won't work. We want to present "There's a new version 1.65.2" before anything else gets printed to stdout. Otherwise it will be quite confusing.

I want to argue that startup time is way more important than printing this banner. We can have a cake and eat it too by doing the check in the background and printing the banner at the top on the subsequent run.

[littledivy]

DenoDir::new is >40% of overall overhead even before the update checker starts up.

Even if DenoDir::new was fully optimized away, this PR will add enough overhead to overpower all optimizations suggested in #15945.

IMO we should reconsider doing this check & display in deno run and move it to some someplace where startup time does not matter (like deno eval / deno).

[piscisaureus]

Was trying to optimize it this morning and I found that this PR does not meaningfully impact startup time.
Something else has made the main branch slower.

piscisaureus@dumbo:~/d/deno 🦕 hyperfine --warmup 2 'target/release/deno run a.js' 'target/release/deno_main run a.js'
Benchmark 1: target/release/deno run a.js
  Time (mean ± σ):      21.9 ms ±   0.4 ms    [User: 21.4 ms, System: 3.9 ms]
  Range (min … max):    21.3 ms …  23.9 ms    124 runs
 
Benchmark 2: target/release/deno_main run a.js
  Time (mean ± σ):      21.8 ms ±   0.3 ms    [User: 21.4 ms, System: 3.9 ms]
  Range (min … max):    21.4 ms …  22.7 ms    123 runs
 
Summary
  'target/release/deno_main run a.js' ran
    1.00 ± 0.02 times faster than 'target/release/deno run a.js'

[piscisaureus]

I want to argue that startup time is way more important than printing this banner. We can have a cake and eat it too by doing the check in the background and printing the banner at the top on the subsequent run.

That's what is implemented in this patch.

[ry]

Main branch vs this one. It appears this one is faster.

# hyperfine "deno run a.js" "./target/release/deno run a.js"
Benchmark 1: deno run a.js
  Time (mean ± σ):      26.5 ms ±   1.4 ms    [User: 21.6 ms, System: 4.4 ms]
  Range (min … max):    24.6 ms …  30.3 ms    95 runs

Benchmark 2: ./target/release/deno run a.js
  Time (mean ± σ):      19.9 ms ±   0.3 ms    [User: 20.8 ms, System: 4.0 ms]
  Range (min … max):    19.5 ms …  21.2 ms    129 runs

Summary
  './target/release/deno run a.js' ran
    1.33 ± 0.07 times faster than 'deno run a.js'

Tho it seems main startup time has gotten much worse than when @littledivy measured it. 26ms vs 13ms.

[littledivy]

@ry that might be because this branch isn't uptodate with main. main had a regression after my measurements.

[bartlomieju]

Latest benchmark:

./third_party/prebuilt/mac/hyperfine --warmup 3 'target/release/deno run a.js' 'deno run a.js'
Benchmark #1: target/release/deno run a.js

  Time (mean ± σ):      16.7 ms ±   0.7 ms    [User: 16.2 ms, System: 6.3 ms]

  Range (min … max):    15.4 ms …  19.5 ms

Benchmark #2: deno run a.js

  Time (mean ± σ):      16.5 ms ±   3.5 ms    [User: 16.4 ms, System: 5.0 ms]

  Range (min … max):    13.8 ms …  33.8 ms

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Summary

  'deno run a.js' ran
    1.01x faster than 'target/release/deno run a.js'

[littledivy]

+bench

[denobot]

⏳ Provisioning metal...

id: 7c95301c-4012-4cfe-a023-5a126b6316c6
metro: unknown

^{_{Use +bench status <id> for status}}

[littledivy]

LGTM

[bartlomieju]

Not finished yet; I'll push changes that limit number of syscalls used today.

[bartlomieju]

+bench

[denobot]

⏳ Provisioning metal...

id: c6db2518-762b-4f53-92b9-acef3561ec6e
metro: unknown

^{_{Use +bench status <id> for status}}

[denobot]

Command	Mean [ms]	Min…Max [ms]
deno run equinix-metal-test/nop.js	13.0 ± 2.4	11.6…20.6
./deno run equinix-metal-test/nop.js	12.4 ± 0.2	12.0…13.0

[bartlomieju]

+bench

[denobot]

⏳ Provisioning metal...

id: 6f04148f-cd14-4ada-bc98-6d402d897c64
metro: unknown

^{_{Use +bench status <id> for status}}

[littledivy]

+bench status 6f04148f-cd14-4ada-bc98-6d402d897c64

[denobot]

✅ Device provisioned 91%

metro: Silicon Valley (US)

[denobot]

Command	Mean [ms]	Min…Max [ms]
deno run equinix-metal-test/nop.js	12.0 ± 0.2	11.5…12.5
./deno run equinix-metal-test/nop.js	12.3 ± 0.3	11.9…14.7

[dsherret]

We should fix how we get the cache directory.

[bartlomieju]

@dsherret addressed the feedback, I changed the checker to only run with deno run (discussed with Ry offline)

[ry]

LGTM

addressed