denoland / deno_core

The core engine at the heart of Deno
MIT License
262 stars 85 forks source link

Road to removal of serde_v8 #716

Open bartlomieju opened 4 months ago

bartlomieju commented 4 months ago

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.

kylewlacy commented 3 months ago

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 commented 2 months ago

@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