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.

Sign up for GitHub

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

Jump to bottom

perf(startup): use WAL journal for sqlite databases in DENO_DIR #23955

Merged
merged 1 commit into from
May 23, 2024
Merged

perf(startup): use WAL journal for sqlite databases in DENO_DIR #23955

merged 1 commit into from
May 23, 2024

Conversation

piscisaureus
Copy link
Member

While investigating poor cold start performance on my GCP VM (32 cores, 130GB SSD), I found that writing to the various sqlite databases in DENO_DIR was quite slow. The slowness seems to primarily be caused by excessive latency from a number of fsync() calls.

The performance difference is best demonstrated by deleting the sqlite databases from DENO_DIR while leaving the downloaded sources in place.

The benchmark (see notes below):

piscisaureus@bert-us:~/erofs/source$ export DENO_DIR=./.deno
piscisaureus@bert-us:~/erofs/source$ hyperfine --warmup 3   \
  --prepare "rm -rf .deno/*_v1*"                            \
  "deno run -A --cached-only demo.ts"                       \
  "eatmydata deno run -A --cached-only demo.ts"             \
  "~/deno/target/release/deno run -A --cached-only demo.ts"
Benchmark 1: deno run -A --cached-only demo.ts
  Time (mean ± σ):      1.174 s ±  0.037 s    [User: 0.153 s, System: 0.184 s]
  Range (min … max):    1.104 s …  1.212 s    10 runs
 
Benchmark 2: eatmydata deno run -A --cached-only demo.ts
  Time (mean ± σ):     265.5 ms ±   3.6 ms    [User: 138.5 ms, System: 135.1 ms]
  Range (min … max):   260.6 ms … 271.2 ms    11 runs
 
Benchmark 3: ~/deno/target/release/deno run -A --cached-only demo.ts
  Time (mean ± σ):     226.2 ms ±   9.2 ms    [User: 136.7 ms, System: 93.3 ms]
  Range (min … max):   218.8 ms … 247.1 ms    13 runs
 
Summary
  ~/deno/target/release/deno run -A --cached-only demo.ts ran
    1.17 ± 0.05 times faster than eatmydata deno run -A --cached-only demo.ts
    5.19 ± 0.27 times faster than deno run -A --cached-only demo.ts

Notes:

  • Benchmark 1: unmodified Deno 1.43.6
  • Benchmark 2: unmodified Deno 1.43.6 wrapped with eatmydata (which is a tool to neuter fsync() calls)
  • Benchmark 3: this PR applied on top of Deno 1.43.6

The script that got benchmarked:

// demo.ts
import * as express from "npm:[email protected]";
import * as postgres from "https://deno.land/x/postgres/mod.ts";

let _dummy = [express, postgres]; // Force use of imports.
console.log("hello world");

@bartlomieju bartlomieju requested a review from dsherret May 23, 2024 00:13
@bartlomieju bartlomieju added this to the 1.44 milestone May 23, 2024
dsherret
dsherret reviewed May 23, 2024
View reviewed changes
cli/cache/cache_db.rs
@@ -42,7 +42,7 @@ impl CacheDBConfiguration {
fn create_combined_sql(&self) -> String {
format!(
"
PRAGMA journal_mode=TRUNCATE;
PRAGMA journal_mode=WAL;
Copy link
Member

Choose a reason for hiding this comment

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

It seems we previously disabled this for performance reasons? #18084

I wonder what impact this will have on non-gcp machines (not sure why that machine is so slow because neither my winows, mac, or linux machine have this issue)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, just realized that was done only for startup perf reasons, but we initialize the sqlite caches on blocking threads now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that was in #18401, and we benchmarked almost all of them within a 10th of a ms.

Copy link
Member

Choose a reason for hiding this comment

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

I just benchmarked WAL and it is much faster on my mac. 176ms down to 142ms.

dsherret
dsherret approved these changes May 23, 2024
View reviewed changes
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

littledivy
littledivy approved these changes May 23, 2024
View reviewed changes
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM!

@dsherret dsherret merged commit de5b47b into main May 23, 2024
17 checks passed
@dsherret dsherret deleted the sqlite_wal branch May 23, 2024 04:33
This was referenced May 23, 2024
Closed
Merged
dsherret added a commit that referenced this pull request May 29, 2024
@dsherret
fix: bump cache sqlite dbs to v2 for WAL journal mode change (#24030)
94f040a 
In #23955 we changed the sqlite db
journal mode to WAL. This causes issues when someone is running an old
version of Deno using TRUNCATE and a new version because the two fight
against each other.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsherret dsherret dsherret approved these changes

@littledivy littledivy littledivy approved these changes

@mmastrac mmastrac Awaiting requested review from mmastrac

@bartlomieju bartlomieju Awaiting requested review from bartlomieju

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.44
Development

Successfully merging this pull request may close these issues.
5 participants
@piscisaureus @mmastrac @dsherret @littledivy @bartlomieju