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

WASM fixes #184

Merged
merged 5 commits into from
Dec 19, 2023
Merged

Conversation

uncomputable
Copy link
Collaborator

Fixes issues that came up while parsing the human encoding in WASM.

@uncomputable uncomputable force-pushed the wasm-fixes branch 3 times, most recently from b557055 to cf11096 Compare December 14, 2023 14:08
@apoelstra
Copy link
Collaborator

thread_local is more efficient than Once because it doesn't need to use synchronization every time that it's accessed.

@apoelstra
Copy link
Collaborator

If WASM can't handle thread_local, even though I wrote the original code specifically with WASM in mind (in which it should be a no-op because WASM has no threads), then I guess it's fine to hack around it by making our code less efficient and unsafe. But I'm not thrilled about it.

@apoelstra
Copy link
Collaborator

I can't test this until we update the vendored version of Simplicity for the recent CI fixes.

@apoelstra
Copy link
Collaborator

Actually, no, I was just building from the wrong system and had an outdated check-pr.nix. I think this is working now.

for (human, value) in human_values {
let s = format!("main := comp const {human} unit");
assert_const::<Core>(s.as_str(), value);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 8355454:

This test fails on my system with

failures:

---- human_encoding::parse::tests::const_word stdout ----
thread 'human_encoding::parse::tests::const_word' panicked at 'byte index 2 is out of bounds of `0`', src/human_encoding/parse/ast.rs:365:47
stack backtrace:
   0: rust_begin_unwind
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_fmt
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/panicking.rs:67:14
   2: core::str::slice_error_fail_rt
   3: core::str::slice_error_fail
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/str/mod.rs:87:9
   4: core::str::traits::<impl core::slice::index::SliceIndex<str> for core::ops::range::Range<usize>>::index
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/str/traits.rs:235:21
   5: core::str::traits::<impl core::ops::index::Index<I> for str>::index
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/str/traits.rs:61:9
   6: simplicity::human_encoding::parse::ast::Ast<J>::from_literal_lexeme
             at ./src/human_encoding/parse/ast.rs:365:47
   7: core::ops::function::Fn::call
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/ops/function.rs:79:5
   8: santiago::parser::tree::Tree<AST>::as_abstract_syntax_tree
             at /store/home/apoelstra/.cargo/registry/src/index.crates.io-6f17d22bba15001f/santiago-1.3.1/src/parser/tree.rs:91:46
   9: simplicity::human_encoding::parse::ast::parse_line_vector
             at ./src/human_encoding/parse/ast.rs:122:11
  10: simplicity::human_encoding::parse::parse
             at ./src/human_encoding/parse/mod.rs:180:17
  11: simplicity::human_encoding::parse::tests::assert_const
             at ./src/human_encoding/parse/mod.rs:642:15
  12: simplicity::human_encoding::parse::tests::const_word
             at ./src/human_encoding/parse/mod.rs:774:13
  13: simplicity::human_encoding::parse::tests::const_word::{{closure}}
             at ./src/human_encoding/parse/mod.rs:751:21
  14: core::ops::function::FnOnce::call_once
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/ops/function.rs:250:5
  15: core::ops::function::FnOnce::call_once
             at /rustc/eb26296b556cef10fb713a38f3d16b9886080f26/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

I think you just need to reorder the commit that introduces tests with the one that fixes parsing of single-character hex literals.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will reorder the commits. I will also try to fix the precomputed powers of two without using Once.

@uncomputable
Copy link
Collaborator Author

For now, I reordered the commits to make the tests work.

apoelstra
apoelstra previously approved these changes Dec 14, 2023
Copy link
Collaborator

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 97e41f5

Option<[Type;N]> is cleaner than [Option<Type>;N], since we initialize
all array values at once.

The line `arr.borrow()[0].is_none()` caused a
`already mutable borrowed: BorrowError` in WASM.

I don't know exactly what caused this error. Maybe it was caused by the
initialize method borrowing POWERS_OF_TWO after nth_power_of_2 borrowed
it. In any case, the error no longer occurs.

The line `two_n = Type::product(two_n.shallow_clone(), two_n)`
caused an addition overflow in WASM.

We compute the next power of two at the beginning of the initialization
loop, preventing the computation of TWO^(2^33) in the final iteration.
The new assertions are more readable and provide better error messages
upon panicking.
The previous code panicked on strings of length 1: half-bytes
Extend the test to cover half bytes etc.
There are forests without the main root.
@uncomputable
Copy link
Collaborator Author

uncomputable commented Dec 19, 2023

thread_local! works, so I added it back in. The borrow error no longer occurs.

We could share the same powers of two across threads using RwLock or OnceLock, but this requires MSRV 1.63.0 and MSRV 1.70.0, respectively, to work in const, so this is for later.

@apoelstra
Copy link
Collaborator

RwLock is extremely slow and should basically never be used unless your problem architecture really requires it. Not sure about OnceLock. It's likely to be better.

Copy link
Collaborator

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK c57d8de

@uncomputable uncomputable merged commit b84cd8c into BlockstreamResearch:master Dec 19, 2023
14 checks passed
@uncomputable uncomputable deleted the wasm-fixes branch December 19, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants