-
Notifications
You must be signed in to change notification settings - Fork 42
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
Deprecated: Add protobuf module as wasm module #2054
base: master
Are you sure you want to change the base?
Conversation
General note: code isn't documented at all. It will be before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this a hug and -at the same time- very clean PR 💯
I had a quick look on the rust part and wrote some comments about the point that caught my eyes. Most of them are points for the final phase if the other performance issues are resolved, and some of them could bring tiny improvements for the performance.
/// * `RangeInclusive` | ||
/// | ||
pub mod common { | ||
include!(concat!(env!("OUT_DIR"), "/common.rs")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if the backslash will be working on Windows, I think we can test that on Windows first before adjusting it manually. Hopefully the rust compiler can resolve automatically.
use crate::*; | ||
use extend::extend; | ||
use prost::Message; | ||
use serde_wasm_bindgen::{from_value, to_value}; | ||
use wasm_bindgen::prelude::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can at some point include the use statements inside the macro itself by writing the full paths to the types inside it.
This would make any future changes easier by changing the macro itself and not having to add using statement for each file at a time.
application/apps/rustcore/rs-bindings/src/js/converting/observe.rs
Outdated
Show resolved
Hide resolved
application/apps/rustcore/rs-bindings/src/js/converting/observe.rs
Outdated
Show resolved
Hide resolved
application/apps/rustcore/rs-bindings/src/js/jobs/converting.rs
Outdated
Show resolved
Hide resolved
5cc8772
to
92fc528
Compare
92fc528
to
94d7357
Compare
94d7357
to
b726d65
Compare
19cab48
to
c3ef80b
Compare
c3ef80b
to
a1f5604
Compare
477be97
to
3e7585f
Compare
4016c2c
to
8b340c1
Compare
a250f65
to
91ea391
Compare
Components
Communication
rustcore
<->rs-binding
: direct calls (Rust context)rs-binding
<->ts-binding
: protobuf messagests-binding
<->holder
: direct calls (NodeJS context)electron
<->client
: IPC messages (JS objects)Issues and the "Vulnerability" of JSON Format
rustcore
exchanges complex objects with deeply nested fields. When working at thers-binding
level, passing and receiving these objects "as is" requires converting them betweenRust
andJS
. This not only complicates the code but may also impact performance. Previously, to avoid this conversion overhead, all objects were transformed intoJSON
strings. However, using theJSON
format can lead to unpredictable results, especially when handling data without a predefined encoding (e.g., invalid UTF-8 characters). Additionally, parsing aJSON
string inJS
andRust
may yield inconsistent results.A potential solution to this problem is to transmit bytes instead of strings and implement a predictable mechanism for encoding and decoding messages from bytes.
Protobuf
was chosen as a candidate protocol due to its ability to define schemas (message lists) and generate encoding/decoding code for bothJS
andRust
.Uncertainty of
protobuf
in Different ContextsA feature of
protobuf
is that the handling of boundary values for fields in a message is determined by the platform implementing the protocol. For example, inRust
, all primitive types have default values, such asu8
defaulting to0
. Thus, for a message likeMsg { field: u8 }
orMsg { field: Option<u8> }
, the following two messages in Rust:Msg { field: 0 }
Msg { field: None }
can be encoded without explicitly defining the
field
value. In the first case, the default value is used, while in the second, the field is undefined. Conversely, in theJS
implementation, the messageMsg { field: 0 }
will always encode thefield
with the value0
, asJS
lacks the concept of default values.As a result, a message like
Msg { field: 0 }
encoded inJS
will consume more bytes than the same message encoded inRust
, making it impossible to decode theRust
-encoded message inJS
.Solution Approaches
One way to address this problem is to define a stricter protocol schema and configure the code generator accordingly. However, this solution is not robust in the long term since schema settings or generator behavior may change. Additionally, stricter schema requirements increase maintenance complexity and hinder code extensibility.
An alternative approach is to use a single
protobuf
implementation for bothRust
andJS
. We can achieve this by encapsulating the generatedprotobuf
code in a trait and wrapping it inwasm
, enabling its usage inJS
without cross-platform compatibility issues. On theRust
side, we can use the crate directly.The only remaining issue is that all messages on the
JS
side remain untyped, meaning all received objects are essentially of typeany
and require additional validation. This issue is partially addressed by the external cratetslink
, which works alongsideprotobuf
code generation to analyze generatedstructs
andenums
and produce*.ts
files with TypeScriptinterfaces
andenums
. This approach ties messages to expected types and allows compile-time checks for message format changes that require code updates. While runtime validation existed in the previousJSON
-based solution, this approach eliminates runtime-only errors and prevents bugs from reaching production releases.Solution
Code Structure
The following additions were made:
Interaction Scheme
The
proto
crate is used directly inrs-binding
for:rustcore
types toprotobuf
messages and vice versa.ts-binding
toprotobuf
messages and then back torustcore
types.This process for communication between
rs-binding
andts-binding
is as follows:It is evident that using
protobuf
under this architecture necessitates an intermediate conversion from bytes toprotobuf
messages and subsequently to "original" types. In contrast, withserde
, original types were directly deserialized from bytes. This tradeoff is the price for adoptingprotobuf
.Notably, all conversion operations are confined to the
rs-binding
andts-binding
components. Other parts of the project remain agnostic toprotobuf
, operating with their native data types.As mentioned earlier, alongside
protobuf
code generation, thetslink
crate generates*.ts
files describingprotobuf
messages in TypeScript terms. These files are placed inapplication/apps/protocol/proto/output
and subsequently copied toapplication/apps/rustcore/ts-bindings/src/protocol
. Thegen.sh
script automates this process.Generation Details
Since the message schema is stable, it does not require frequent regeneration for every release or code change (unless the
protobuf
schema itself changes). Furthermore, the generated code is platform-independent, eliminating the need for separate generation per platform. Thus, incorporatingprotobuf
generation into the build system is unnecessary. A simple script (application/apps/protocol/gen.sh
) suffices for updating the code as needed.Conclusion
The main achievement in this task is leveraging
wasm
to improve not only performance but also stability. As discussed,protobuf
implementations inJS
andRust
are incompatible in certain "edge" cases. However, usingwasm
elegantly resolves this issue while enhancing performance.Nonetheless, adopting
protobuf
introduces complexity due to the required conversion ofprotobuf
messages into original types. Given that this communication is entirely internal (i.e., no external network messages necessitate strict adherence to a protocol),protobuf
might be an over-engineered solution. A simpler alternative usingserde
+bincode
(withwasm
integration) could suffice.Finally, during this task, a bug in
node-bindgen
was identified and resolved. The crate lacked safe mechanisms for sending/receiving bytes, which has now been addressed.