Support pulley applets using Wasmtime#907
Conversation
ia0
added
crate:cli
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Pulley applets using Wasmtime, which is a significant and well-executed addition. The changes are comprehensive, touching the CLI tools, scheduler, and CI scripts to properly integrate the new backend. The implementation of the Pulley store in the scheduler is particularly noteworthy, showcasing a deep understanding of Wasmtime's async capabilities. The refactoring of the WASM optimization logic is also a good improvement for maintainability. I have one minor suggestion regarding cleanup of temporary files to improve the build process hygiene.
4f0658c to
d5599bf
Compare
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Pulley applets using Wasmtime, which is a significant enhancement for performance and footprint. The changes are extensive, touching the command-line tools, the scheduler, and various configuration and test files. The core of the implementation is in the new crates/scheduler/src/applet/store/pulley.rs file. My review has focused on the correctness, robustness, and performance of this new backend. I've identified a critical issue that could lead to a panic in the scheduler, as well as several high-severity issues related to unsafe code, potential panics from unhandled errors, and performance concerns due to allocations in hot paths. I've also noted a design limitation in the applet initialization sequence.
b51d490 to
3d36cc3
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Pulley applets using Wasmtime, which is a significant enhancement for performance and footprint on embedded platforms. The changes are extensive, touching upon memory layout adjustments, build tooling, and the scheduler's core logic. The implementation is well-structured, and the necessary updates to test scripts and documentation are included. I've identified a potential panic in the scheduler logic and a couple of areas for improvement regarding performance and documentation clarity.
ia0-review
left a comment
ia0-review
left a comment
There was a problem hiding this comment.
It should be possible to merge some of the wasm and pulley code, but can probably be done later.
2 participants
The pulley platform is able to run the OpenSK applet and the BLE advertisement sniffer applet on Nordic. However, the footprint is significative and extremely close to the maximum, which has been increased:
From the 303.5KiB
.textsize of the pulley platform, we have 84.2KiB for wasmtime, 59.2KiB for wasefire-scheduler, 53.5KiB for pulley-interpreter, 18.2KiB for core, and the rest is smaller than 11KiB each for a remaining size of 88.4KiB. The opensk applet is 262.1KiB.For comparison, from the 146.4KiB
.textsize of the native opensk platform, we have 54.4KiB for opensk, 22.7KiB for wasefire-scheduler, 9.9KiB for core, and the rest is smaller than 8KiB each for a remaining size of 59.4KiB. But this comparison is unfair because much more dead code can be eliminated with a native applet.Instead we can compare with the 128.5KiB
.textsize of the wasm platform, where we 40.8KiB for the wasefire-scheduler, 31.9KiB for the wasefire-interpreter, and the rest is smaller than 8KiB each for a remaining size of 55.8KiB.Regarding RAM, the pulley platform (with opensk applet) peaks at 163.4KiB. The native platform peaks at 3.7KiB. At least 64KiB are due to the applet memory. We also have 16KiB for async stack and 8KiB for wasm stack configurations of Wasmtime.
Timings between native and pulley on CTAP1 register and authenticate is very similar (within noise levels): 1.3760 and 0.7201 respectively for native and 1.3638 and 0.7240 for pulley. This might be somewhat expected since most of the time should be spent in software crypto which is native.
Timings between wasm and pulley for the BLE advertisement applet is significative. The pulley applet doesn't drop any package in a somewhat BLE noisy environment, while the wasm applet is constantly dropping applets. The expected performance improvement is a factor 20 based on the wasm-bench tests (minimal coremark).
Fixes #458