feat(ops): Fast zero copy string arguments #16777

littledivy · 2022-11-23T13:26:19Z

Uses SeqOneByteString optimization to do zero-copy &str arguments in fast calls.

Depends on Add SeqOneByteString to fast API calls rusty_v8#1129
- Depends on https://chromium-review.googlesource.com/c/v8/v8/+/4036884
Disable in async ops
Make it work with owned String with an extra alloc in fast path.
Support Cow<'_, str>. Owned for slow case, Borrowed for fast case

#[op]
fn op_string_len(s: &str) -> u32 { 
  str.len() as u32 
}

aapoalas · 2022-11-23T13:53:28Z

I recommend testing some Latin-1 (V8's OneByte) and UTF-8 clashing characters, eg. © ("\u00A9") is a good one.

EDIT: Not sure if that's actually a clashing character... But at least I've recently been bitten by presuming that V8's OneByteStrings are directly usable as UTF-8, specifically with the copyright symbol as the test character that broke.

… 'main' of github.com:denoland/deno into fast-string-args

bartlomieju · 2022-12-01T12:42:14Z

I recommend testing some Latin-1 (V8's OneByte) and UTF-8 clashing characters, eg. © ("\u00A9") is a good one.

EDIT: Not sure if that's actually a clashing character... But at least I've recently been bitten by presuming that V8's OneByteStrings are directly usable as UTF-8, specifically with the copyright symbol as the test character that broke.

@littledivy did you add such a test?

cli/bench/console.js

littledivy · 2022-12-01T14:22:44Z

@bartlomieju There are tests for that in the V8 patch: https://chromium-review.googlesource.com/c/v8/v8/+/4036884

ry · 2022-12-01T13:59:14Z

ext/web/lib.rs

 fn op_encoding_encode_into(
-  scope: &mut v8::HandleScope,
-  input: serde_v8::Value,
+  input: Cow<'_, str>,


Why not &str?

ry · 2022-12-01T14:01:09Z

ext/web/lib.rs

+    // ops guarantee this to be a SeqOneByteString.
+    Cow::Borrowed(v) => v[..boundary].len() as u32,
+    Cow::Owned(v) => v[..boundary].encode_utf16().count() as u32,


Cow::Borrowed means SeqOneByteString and Cow::Owned means utf16? I think a more verbose comment here is warranted.

+1 please explain which variant should be used in which situation

ext/web/lib.rs

ry · 2022-12-01T14:06:36Z

ops/lib.rs

+fn is_string(ty: impl ToTokens) -> Option<bool> {
+  let toks = tokens(ty);
+  if toks == "String" {
+    return Some(false);
+  }
+  if toks == "& str" {
+    return Some(true);
+  }
+  None
 }

 fn is_option_string(ty: impl ToTokens) -> bool {
  tokens(ty) == "Option < String >"
 }

+fn is_cow_str(ty: impl ToTokens) -> bool {
+  tokens(&ty).starts_with("Cow <") && tokens(&ty).ends_with("str >")
+}
+


is_string shouldn't also include Cow strings? Maybe it should be called is_non_cow_string?

It would be nice to have unit tests for these little utility functions. Is that hard?

ry

LGTM - some comments...

awesome work @littledivy - this is super cool

This reverts commit 9b2b8df.

This reverts commit 9b2b8df. Closes dsherret/ts-morph#1372 Closes #16979

Reland #16777 The codegen is disabled in async ops and when fallback to slow call is possible (return type is a Result) to avoid hitting this V8 bug: #17159

feat(ops): Add fast zero copy string arguments

7fc52b6

littledivy changed the title ~~feat(ops): Add fast zero copy string arguments~~ feat(ops): Fast zero copy string arguments Nov 23, 2022

also apply to owned Strings

8539096

littledivy added 11 commits November 24, 2022 09:24

disable ref strings in async ops

f0e4675

Fix

0b86279

Merge branch 'fast-string-args' of github.com:littledivy/deno; branch…

c383d6d

… 'main' of github.com:denoland/deno into fast-string-args

x

6322974

stuff

fdaa2a0

more stuff

ca4ba7f

url

0fa912c

Cow<'_, str>

5611535

x

1184fa4

Merge branch 'main' of github.com:denoland/deno into fast-string-args

6a7ca06

lint deno_ops

071b98f

littledivy marked this pull request as ready for review December 1, 2022 04:15

littledivy requested review from bartlomieju, piscisaureus and ry December 1, 2022 04:33

ry reviewed Dec 1, 2022

View reviewed changes

cli/bench/console.js Show resolved Hide resolved

ry reviewed Dec 1, 2022

View reviewed changes

ry approved these changes Dec 1, 2022

View reviewed changes

example that bit a little more

bc61bf0

littledivy enabled auto-merge (squash) December 2, 2022 05:02

littledivy merged commit 9b2b8df into denoland:main Dec 2, 2022

bartlomieju added a commit to bartlomieju/deno that referenced this pull request Dec 15, 2022

Revert "feat(ops): Fast zero copy string arguments (denoland#16777)"

ee9d78c

This reverts commit 9b2b8df.

dsherret pushed a commit that referenced this pull request Dec 15, 2022

Revert "feat(ops): Fast zero copy string arguments (#16777)" (#17063)

585ec12

This reverts commit 9b2b8df. Closes dsherret/ts-morph#1372 Closes #16979

littledivy mentioned this pull request Mar 2, 2023

feat(ops): reland fast zero copy string arguments #17996

Merged

kt3k pushed a commit that referenced this pull request Mar 10, 2023

feat(ops): reland fast zero copy string arguments (#17996)

eda8c83

Reland #16777 The codegen is disabled in async ops and when fallback to slow call is possible (return type is a Result) to avoid hitting this V8 bug: #17159

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ops): Fast zero copy string arguments #16777

feat(ops): Fast zero copy string arguments #16777

littledivy commented Nov 23, 2022 •

edited

Loading

aapoalas commented Nov 23, 2022 •

edited

Loading

bartlomieju commented Dec 1, 2022

littledivy commented Dec 1, 2022

ry Dec 1, 2022

ry Dec 1, 2022

bartlomieju Dec 1, 2022

ry Dec 1, 2022

ry left a comment •

edited

Loading

feat(ops): Fast zero copy string arguments #16777

feat(ops): Fast zero copy string arguments #16777

Conversation

littledivy commented Nov 23, 2022 • edited Loading

aapoalas commented Nov 23, 2022 • edited Loading

bartlomieju commented Dec 1, 2022

littledivy commented Dec 1, 2022

ry Dec 1, 2022

Choose a reason for hiding this comment

ry Dec 1, 2022

Choose a reason for hiding this comment

bartlomieju Dec 1, 2022

Choose a reason for hiding this comment

ry Dec 1, 2022

Choose a reason for hiding this comment

ry left a comment • edited Loading

Choose a reason for hiding this comment

littledivy commented Nov 23, 2022 •

edited

Loading

aapoalas commented Nov 23, 2022 •

edited

Loading

ry left a comment •

edited

Loading