clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-6355] Speed up CLI startup by ~35% by lazy-loading `DOMParser`

Open jack-weilage opened this issue 1 year ago • 5 comments

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Bitwarden's CLI takes ~2.5s to run bw --version on my laptop. When poking around in bw.ts, I noticed that an instance of JSDOM was created every time the CLI starts up, even though it's rarely used. If that polyfill is lazily loaded, the CLI starts up ~500ms faster in all circumstances. When running bw --version, thats ~37% faster!

There's quite a few places to speed up the CLI, and I'll be looking into solutions for them in the near future. In a quickly hacked together version, I got about 2x startup time just by lazy-loading dependencies!

Benchmark 1: node new/bw.js --version
  Time (mean ± σ):      1.772 s ±  0.018 s    [User: 1.861 s, System: 0.212 s]
  Range (min … max):    1.752 s …  1.802 s    10 runs
 
Benchmark 2: node old/bw.js --version
  Time (mean ± σ):      2.439 s ±  0.013 s    [User: 2.587 s, System: 0.254 s]
  Range (min … max):    2.423 s …  2.466 s    10 runs
 
Summary
  node new/bw.js --version ran
    1.38 ± 0.02 times faster than node old/bw.js --version

Code changes

  • apps/cli/src/bw.ts:
    • I replaced import * as jsdom with import { JSDOM }, as the CLI never uses anything else from jsdom, although it's only used for typing now.
    • I replaced the old DOMParser polyfill (which just immediately created a JSDOM instance) with a lazy-loaded one (which calls new JSDOM only when global.DOMParser is accessed).

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

jack-weilage avatar Feb 18 '24 02:02 jack-weilage

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 18 '24 02:02 CLAassistant

Thank you for your contribution! We've added this to our internal Community PR board for review. ID: PM-6355

bitwarden-bot avatar Feb 18 '24 02:02 bitwarden-bot

Logo Checkmarx One – Scan Summary & Details817940c7-31fe-468b-bea8-8d4aa589292d

No New Or Fixed Issues Found

bitwarden-bot avatar Feb 18 '24 03:02 bitwarden-bot

I've been shifting around dependencies for a while, and I've gotten the startup time ~3.6x faster. All tests are passing and no dependencies/logic have been changed.

I have some questions about the code style/normal practices for Bitwarden:

  1. Is there a reason that import type calls are not used when something is only imported as a type?
  2. Are require() or import() statements preferred when dynamically importing modules? tsconfig.eslint.json has module: commonjs, so TypeScript complains when using import(), but @typescript-eslint/no-var-requires complains when require() is used.

Benchmark (with all changes)

Benchmark 1: node build/bw.js --version
  Time (mean ± σ):     369.4 ms ±  29.2 ms    [User: 364.7 ms, System: 59.0 ms]
  Range (min … max):   358.2 ms … 452.4 ms    10 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 2: node build.bak/bw.js --version
  Time (mean ± σ):     968.1 ms ±  31.4 ms    [User: 1019.7 ms, System: 115.7 ms]
  Range (min … max):   949.4 ms … 1053.8 ms    10 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Summary
  node build/bw.js --version ran
    3.62 ± 0.22 times faster than node build.bak/bw.js --version

Flamegraphs

Before image

After image

jack-weilage avatar Feb 18 '24 21:02 jack-weilage

Related to https://github.com/bitwarden/cli/pull/530

djsmith85 avatar Feb 19 '24 09:02 djsmith85