perf(ext/webstorage): use object wrap for Storage (#26931)

![image](https://github.com/user-attachments/assets/3f86e2fd-9026-4965-8f3b-512423362f1e) Depends on: - https://github.com/denoland/deno_core/pull/970 - https://github.com/denoland/deno_core/pull/976 - https://github.com/denoland/deno_core/pull/980 - https://github.com/denoland/deno_core/pull/981 --------- Signed-off-by: Divy Srivastava <dj.srivastava23@gmail.com>
2024-11-30 16:40:57 -05:00 · 2024-11-27 02:41:57 -08:00 · 2024-11-27 02:41:57 -08:00 · 1e51b650be
commit 1e51b650be
parent 7400181ecb
5 changed files with 132 additions and 182 deletions
--- a/Cargo.lock
+++ b/Cargo.lock
@ -394,6 +394,12 @@ dependencies = [
 "tower-service",
 ]

+[[package]]
+name = "az"
+version = "1.2.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7b7e4c2464d97fe331d41de9d5db0def0a96f4d823b8b32a2efd503578988973"
+
 [[package]]
 name = "backtrace"
 version = "0.3.73"
@ -1454,11 +1460,12 @@ dependencies = [

 [[package]]
 name = "deno_core"
-version = "0.322.0"
+version = "0.323.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "2f593ef2b8acab8cd3ace9d50052edc65a3654fdbde808070cfa5da5cf7aaae6"
+checksum = "a781bcfe1b5211b8497f45bf5b3dba73036b8d5d1533c1f05d26ccf0afb25a78"
 dependencies = [
 "anyhow",
+ "az",
 "bincode",
 "bit-set",
 "bit-vec",
@ -1969,9 +1976,9 @@ dependencies = [

 [[package]]
 name = "deno_ops"
-version = "0.198.0"
+version = "0.199.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "870826735cd9aa0376d2aadca14365b753e830e3cc16891efb9232845a6982a4"
+checksum = "a24a1f3e22029a57d3094b32070b8328eac793920b5a022027d360f085e6b245"
 dependencies = [
 "proc-macro-rules",
 "proc-macro2",
@ -6570,9 +6577,9 @@ dependencies = [

 [[package]]
 name = "serde_v8"
-version = "0.231.0"
+version = "0.232.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1a0c48b8842ebae21c52da1d978fba5c2be5991680bddfdc1a36ee0ccbc60114"
+checksum = "5c9feae92f7293fcc1a32a86be1a399859c0637e55dad8991d5258c43f7ff4d2"
 dependencies = [
 "num-bigint",
 "serde",
--- a/Cargo.toml
+++ b/Cargo.toml
@ -47,7 +47,7 @@ repository = "https://github.com/denoland/deno"

 [workspace.dependencies]
 deno_ast = { version = "=0.43.3", features = ["transpiling"] }
-deno_core = { version = "0.322.0" }
+deno_core = { version = "0.323.0" }

 deno_bench_util = { version = "0.173.0", path = "./bench_util" }
 deno_config = { version = "=0.39.2", features = ["workspace", "sync"] }
--- a/ext/webstorage/01_webstorage.js
+++ b/ext/webstorage/01_webstorage.js
@ -3,91 +3,20 @@
 /// <reference path="../../core/internal.d.ts" />

 import { primordials } from "ext:core/mod.js";
-import {
-  op_webstorage_clear,
-  op_webstorage_get,
-  op_webstorage_iterate_keys,
-  op_webstorage_key,
-  op_webstorage_length,
-  op_webstorage_remove,
-  op_webstorage_set,
-} from "ext:core/ops";
+import { op_webstorage_iterate_keys, Storage } from "ext:core/ops";
 const {
-  Symbol,
  SymbolFor,
  ObjectFromEntries,
  ObjectEntries,
  ReflectDefineProperty,
  ReflectDeleteProperty,
-  ReflectGet,
+  FunctionPrototypeBind,
  ReflectHas,
  Proxy,
 } = primordials;

-import * as webidl from "ext:deno_webidl/00_webidl.js";
-
-const _persistent = Symbol("[[persistent]]");
-
-class Storage {
-  [_persistent];
-
-  constructor() {
-    webidl.illegalConstructor();
-  }
-
-  get length() {
-    webidl.assertBranded(this, StoragePrototype);
-    return op_webstorage_length(this[_persistent]);
-  }
-
-  key(index) {
-    webidl.assertBranded(this, StoragePrototype);
-    const prefix = "Failed to execute 'key' on 'Storage'";
-    webidl.requiredArguments(arguments.length, 1, prefix);
-    index = webidl.converters["unsigned long"](index, prefix, "Argument 1");
-
-    return op_webstorage_key(index, this[_persistent]);
-  }
-
-  setItem(key, value) {
-    webidl.assertBranded(this, StoragePrototype);
-    const prefix = "Failed to execute 'setItem' on 'Storage'";
-    webidl.requiredArguments(arguments.length, 2, prefix);
-    key = webidl.converters.DOMString(key, prefix, "Argument 1");
-    value = webidl.converters.DOMString(value, prefix, "Argument 2");
-
-    op_webstorage_set(key, value, this[_persistent]);
-  }
-
-  getItem(key) {
-    webidl.assertBranded(this, StoragePrototype);
-    const prefix = "Failed to execute 'getItem' on 'Storage'";
-    webidl.requiredArguments(arguments.length, 1, prefix);
-    key = webidl.converters.DOMString(key, prefix, "Argument 1");
-
-    return op_webstorage_get(key, this[_persistent]);
-  }
-
-  removeItem(key) {
-    webidl.assertBranded(this, StoragePrototype);
-    const prefix = "Failed to execute 'removeItem' on 'Storage'";
-    webidl.requiredArguments(arguments.length, 1, prefix);
-    key = webidl.converters.DOMString(key, prefix, "Argument 1");
-
-    op_webstorage_remove(key, this[_persistent]);
-  }
-
-  clear() {
-    webidl.assertBranded(this, StoragePrototype);
-    op_webstorage_clear(this[_persistent]);
-  }
-}
-
-const StoragePrototype = Storage.prototype;
-
 function createStorage(persistent) {
-  const storage = webidl.createBranded(Storage);
-  storage[_persistent] = persistent;
+  const storage = new Storage(persistent);

  const proxy = new Proxy(storage, {
    deleteProperty(target, key) {
@ -106,12 +35,16 @@ function createStorage(persistent) {
      return true;
    },

-    get(target, key, receiver) {
+    get(target, key) {
      if (typeof key === "symbol") {
        return target[key];
      }
      if (ReflectHas(target, key)) {
-        return ReflectGet(target, key, receiver);
+        const value = target[key];
+        if (typeof value === "function") {
+          return FunctionPrototypeBind(value, target);
+        }
+        return value;
      }
      return target.getItem(key) ?? undefined;
    },
@ -136,7 +69,7 @@ function createStorage(persistent) {
    },

    ownKeys() {
-      return op_webstorage_iterate_keys(persistent);
+      return op_webstorage_iterate_keys(storage);
    },

    getOwnPropertyDescriptor(target, key) {
@ -163,7 +96,7 @@ function createStorage(persistent) {
    inspect,
    inspectOptions,
  ) {
-    return `${this.constructor.name} ${
+    return `Storage ${
      inspect({
        ...ObjectFromEntries(ObjectEntries(proxy)),
        length: this.length,
--- a/ext/webstorage/lib.rs
+++ b/ext/webstorage/lib.rs
@ -5,6 +5,7 @@
 use std::path::PathBuf;

 use deno_core::op2;
+use deno_core::GarbageCollected;
 use deno_core::OpState;
 use rusqlite::params;
 use rusqlite::Connection;
@ -32,17 +33,14 @@ const MAX_STORAGE_BYTES: usize = 10 * 1024 * 1024;
 deno_core::extension!(deno_webstorage,
  deps = [ deno_webidl ],
  ops = [
-    op_webstorage_length,
-    op_webstorage_key,
-    op_webstorage_set,
-    op_webstorage_get,
-    op_webstorage_remove,
-    op_webstorage_clear,
    op_webstorage_iterate_keys,
  ],
+  objects = [
+    Storage
+  ],
  esm = [ "01_webstorage.js" ],
  options = {
-    origin_storage_dir: Option<PathBuf>
+      origin_storage_dir: Option<PathBuf>
  },
  state = |state, options| {
    if let Some(origin_storage_dir) = options.origin_storage_dir {
@ -110,38 +108,6 @@ fn get_webstorage(
  Ok(conn)
 }

-#[op2(fast)]
-pub fn op_webstorage_length(
-  state: &mut OpState,
-  persistent: bool,
-) -> Result<u32, WebStorageError> {
-  let conn = get_webstorage(state, persistent)?;
-
-  let mut stmt = conn.prepare_cached("SELECT COUNT(*) FROM data")?;
-  let length: u32 = stmt.query_row(params![], |row| row.get(0))?;
-
-  Ok(length)
-}
-
-#[op2]
-#[string]
-pub fn op_webstorage_key(
-  state: &mut OpState,
-  #[smi] index: u32,
-  persistent: bool,
-) -> Result<Option<String>, WebStorageError> {
-  let conn = get_webstorage(state, persistent)?;
-
-  let mut stmt =
-    conn.prepare_cached("SELECT key FROM data LIMIT 1 OFFSET ?")?;
-
-  let key: Option<String> = stmt
-    .query_row(params![index], |row| row.get(0))
-    .optional()?;
-
-  Ok(key)
-}
-
 #[inline]
 fn size_check(input: usize) -> Result<(), WebStorageError> {
  if input >= MAX_STORAGE_BYTES {
@ -151,81 +117,125 @@ fn size_check(input: usize) -> Result<(), WebStorageError> {
  Ok(())
 }

-#[op2(fast)]
-pub fn op_webstorage_set(
-  state: &mut OpState,
-  #[string] key: &str,
-  #[string] value: &str,
+struct Storage {
  persistent: bool,
-) -> Result<(), WebStorageError> {
-  let conn = get_webstorage(state, persistent)?;
-
-  size_check(key.len() + value.len())?;
-
-  let mut stmt = conn
-    .prepare_cached("SELECT SUM(pgsize) FROM dbstat WHERE name = 'data'")?;
-  let size: u32 = stmt.query_row(params![], |row| row.get(0))?;
-
-  size_check(size as usize)?;
-
-  let mut stmt = conn
-    .prepare_cached("INSERT OR REPLACE INTO data (key, value) VALUES (?, ?)")?;
-  stmt.execute(params![key, value])?;
-
-  Ok(())
 }

+impl GarbageCollected for Storage {}
+
 #[op2]
-#[string]
-pub fn op_webstorage_get(
-  state: &mut OpState,
-  #[string] key_name: String,
-  persistent: bool,
-) -> Result<Option<String>, WebStorageError> {
-  let conn = get_webstorage(state, persistent)?;
+impl Storage {
+  #[constructor]
+  #[cppgc]
+  fn new(persistent: bool) -> Storage {
+    Storage { persistent }
+  }

-  let mut stmt = conn.prepare_cached("SELECT value FROM data WHERE key = ?")?;
-  let val = stmt
-    .query_row(params![key_name], |row| row.get(0))
-    .optional()?;
+  #[getter]
+  #[smi]
+  fn length(&self, state: &mut OpState) -> Result<u32, WebStorageError> {
+    let conn = get_webstorage(state, self.persistent)?;

-  Ok(val)
-}
+    let mut stmt = conn.prepare_cached("SELECT COUNT(*) FROM data")?;
+    let length: u32 = stmt.query_row(params![], |row| row.get(0))?;

-#[op2(fast)]
-pub fn op_webstorage_remove(
-  state: &mut OpState,
-  #[string] key_name: &str,
-  persistent: bool,
-) -> Result<(), WebStorageError> {
-  let conn = get_webstorage(state, persistent)?;
+    Ok(length)
+  }

-  let mut stmt = conn.prepare_cached("DELETE FROM data WHERE key = ?")?;
-  stmt.execute(params![key_name])?;
+  #[required(1)]
+  #[string]
+  fn key(
+    &self,
+    state: &mut OpState,
+    #[smi] index: u32,
+  ) -> Result<Option<String>, WebStorageError> {
+    let conn = get_webstorage(state, self.persistent)?;

-  Ok(())
-}
+    let mut stmt =
+      conn.prepare_cached("SELECT key FROM data LIMIT 1 OFFSET ?")?;

-#[op2(fast)]
-pub fn op_webstorage_clear(
-  state: &mut OpState,
-  persistent: bool,
-) -> Result<(), WebStorageError> {
-  let conn = get_webstorage(state, persistent)?;
+    let key: Option<String> = stmt
+      .query_row(params![index], |row| row.get(0))
+      .optional()?;

-  let mut stmt = conn.prepare_cached("DELETE FROM data")?;
-  stmt.execute(params![])?;
+    Ok(key)
+  }

-  Ok(())
+  #[fast]
+  #[required(2)]
+  fn set_item(
+    &self,
+    state: &mut OpState,
+    #[string] key: &str,
+    #[string] value: &str,
+  ) -> Result<(), WebStorageError> {
+    let conn = get_webstorage(state, self.persistent)?;
+
+    size_check(key.len() + value.len())?;
+
+    let mut stmt = conn
+      .prepare_cached("SELECT SUM(pgsize) FROM dbstat WHERE name = 'data'")?;
+    let size: u32 = stmt.query_row(params![], |row| row.get(0))?;
+
+    size_check(size as usize)?;
+
+    let mut stmt = conn.prepare_cached(
+      "INSERT OR REPLACE INTO data (key, value) VALUES (?, ?)",
+    )?;
+    stmt.execute(params![key, value])?;
+
+    Ok(())
+  }
+
+  #[required(1)]
+  #[string]
+  fn get_item(
+    &self,
+    state: &mut OpState,
+    #[string] key: &str,
+  ) -> Result<Option<String>, WebStorageError> {
+    let conn = get_webstorage(state, self.persistent)?;
+
+    let mut stmt =
+      conn.prepare_cached("SELECT value FROM data WHERE key = ?")?;
+    let val = stmt.query_row(params![key], |row| row.get(0)).optional()?;
+
+    Ok(val)
+  }
+
+  #[fast]
+  #[required(1)]
+  fn remove_item(
+    &self,
+    state: &mut OpState,
+    #[string] key: &str,
+  ) -> Result<(), WebStorageError> {
+    let conn = get_webstorage(state, self.persistent)?;
+
+    let mut stmt = conn.prepare_cached("DELETE FROM data WHERE key = ?")?;
+    stmt.execute(params![key])?;
+
+    Ok(())
+  }
+
+  #[fast]
+  fn clear(&self, state: &mut OpState) -> Result<(), WebStorageError> {
+    let conn = get_webstorage(state, self.persistent)?;
+
+    let mut stmt = conn.prepare_cached("DELETE FROM data")?;
+    stmt.execute(params![])?;
+
+    Ok(())
+  }
 }

 #[op2]
 #[serde]
-pub fn op_webstorage_iterate_keys(
+fn op_webstorage_iterate_keys(
+  #[cppgc] storage: &Storage,
  state: &mut OpState,
-  persistent: bool,
 ) -> Result<Vec<String>, WebStorageError> {
-  let conn = get_webstorage(state, persistent)?;
+  let conn = get_webstorage(state, storage.persistent)?;

  let mut stmt = conn.prepare_cached("SELECT key FROM data")?;
  let keys = stmt
--- a/tests/wpt/runner/expectation.json
+++ b/tests/wpt/runner/expectation.json
@ -9789,7 +9789,7 @@
    "event_constructor.window.html": false,
    "event_initstorageevent.window.html": false,
    "missing_arguments.window.html": true,
-    "storage_builtins.window.html": true,
+    "storage_builtins.window.html": false,
    "storage_clear.window.html": true,
    "storage_functions_not_overwritten.window.html": true,
    "storage_getitem.window.html": true,