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.

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

Road to removal of serde_v8 #716

Open
bartlomieju opened this issue Apr 23, 2024 · 2 comments
Open

Road to removal of serde_v8 #716

bartlomieju opened this issue Apr 23, 2024 · 2 comments
Assignees
Labels
ops op functionality

Comments

@bartlomieju
Copy link
Member

Discussed extensively with @nathanwhit @mmastrac and @dsherret.

We are planning to remove serde_v8 completely. The reason is that in most cases using serde_v8 is a big footgun in terms of performance.

A stark example of perf footgun is URLPattern API that uses ops returning structures that are marked with #[serde]. This API produces a lot of garbage, just because it returns objects that have 8-9 keys. On each op invocation the key is serialized and copied from Rust to V8. Then in JavaScript, this object is immediately destructured and GCed. Instead we could return an array of values, which would alleviate a lot of pressure on GC.

This will need to happen over multiple PRs. The tentative plan of action is:

  1. Remove serde_v8::AnyValue - this enum is used only for Deno KV APIs and can be easily replaced by using v8::Value directly.
  2. Once AnyValue is gone, we can remove support for #[serde] arg in #[op2]. This is a big antipattern to pass structs as arguments - instead, the fields should be "unfurled" and passed as separate arguments to ops.
  3. Remove support for other "magic" structs like JsBuffer, StringOrBuffer, BigInt, etc. Most of these could use specialized attributes or implement ToV8 and FromV8 traits directly.
  4. At this point we should be able to remove #[serde] support in return value. Instead of returning structs we'd be returning v8::Array or v8::Value and it would be up to the caller to construct an object on JS side (if it's even needed). After checking deno repo we don't have that many occurrences and should be fairly straightfoward to update.
  5. At this point, we should do the serde_v8 release saying that it's deprecated and unmaintained.
  6. Remove serde_v8 crate altogether.

In the meantime we can remove serde_v8 benchmarks that will speed up CI in deno_core.

@bartlomieju bartlomieju added the ops op functionality label Apr 23, 2024
bartlomieju added a commit that referenced this issue May 16, 2024
As per #716 we are working
on replacing `serde_v8` with a new mechanism completely.

To that end, running benchmarks now is not that useful and only adds to
the overall CI time.
@kylewlacy
Copy link

For Brioche, I use serde_v8 for quite a few of the important ops in the runtime. The data structures are fairly big and often deeply-nested, so I don't think translating these structures to a regular list of primitive arguments would be feasible

I could just use JSON.stringify() / serde_json::from_str() on either side, or I'm sure I could write some manual de/serialization for v8::Value types, but is there some general guidance or pattern to avoid either of these options?

@rscarson
Copy link
Contributor

rscarson commented Jul 12, 2024

@bartlomieju

Removal of serde_v8 would entirely break a lot of paths from v8 types to rust types

It would entirely, completely break rustyscript

I would urge you guys to reconsider. It is very easy to misuse a lot of things, but that is not a reason to remove them entirely imho

Please consider leaving serde_v8 in, so as not to catastrophically break a -LOT- of downstream code.

Better documentation, warnings, or even feature-gating are all far better options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ops op functionality
Projects
None yet
Development

No branches or pull requests

4 participants