Rust-based Dialect api. #109

richardkiss · 2021-08-23T21:21:06Z

No description provided.

arvidn

I think, generally, a single interface/trait would suffice and make this API simpler. I think Dialects should be that trait that then has two implementations, one ChiaDialect which does not keep a lookup table for functions, and maybe the other one could be called CustomDialect, or something like that, which is configurable via a string map.

arvidn · 2021-08-24T05:36:29Z

src/chia_dialect.rs

+const QUOTE_KW: [u8; 1] = [1];
+const APPLY_KW: [u8; 1] = [2];
+
+pub struct OperatorHandlerWithMode {


this doesn't look like it belongs in the chia dialect. The chia dialect is known and doesn't need to be determined at run-time. It doesn't need to keep a run-time lookup-table. In fact, the run-time lookup-table seems like it belongs in the python binding, as it's a trick to allow mixing python functions with rust functions. In the rust API, there's no need for that.

The lowest-level run_program implementation currently expects a thing with the OperatorHandler trait, which defines the opcode-to-functionality map. Except for strict, this is pretty much the simplest implementation of that (FLookup is just an array of 256 optional function pointers).

I can move this to its own file.

not as simple as this, which is stateless: https://github.com/Chia-Network/clvm_rs/pull/107/files#diff-4f84c6fa87ad47416016af253147484a8eef4a052dd794d4fd144db78865a172R33

arvidn · 2021-08-24T05:37:04Z

src/chia_dialect.rs

+    OperatorHandlerWithMode::new_with_hashmap(chia_opcode_mapping(), strict)
+}
+
+pub fn chia_dialect(strict: bool) -> Dialect {


as far as I can tell, this is the only function that needs to be public in this file.

arvidn · 2021-08-24T05:39:13Z

src/dialect.rs

+pub struct Dialect {
+    quote_kw: Vec<u8>,
+    apply_kw: Vec<u8>,
+    op_handler: Box<dyn OperatorHandler>,


I don't see why this needs to be a run-time interface instead of a compile-time trait, not why it needs to be heap allocated. In the chia dialect case this class only contains one bool of state (strict mode).

Dialect seems like a perfect candidate for being a trait rather than a concrete type.

I could make a dialect trait, but we'd still need at least one concrete instance.

Changing to op_handler: &dyn OperatorHandler would mean having to keep around both a Dialect and the concrete OperatorHandler.

what I'm suggesting is that you can just rename the OperatorHandler trait to Dialect and add the two quote_kw and apply_kw to it. Then the python binding can have a concrete implementation of this trait, which contains a run-time lookup table, and the chia_rs library could have its own concrete type that doesn't, but that has a static operator -> function mapping.

arvidn · 2021-08-24T05:40:31Z

src/dialect.rs

+        args: NodePtr,
+        max_cost: Cost,
+    ) -> Response {
+        self.op_handler.op(allocator, op, args, max_cost)


instead of forwarding this call, could the Dialect trait and OperatorHandler traits just be merged? They seem simple enough and related enough that it could just be one Dialect trait

For sure that'd be possible, but I don't immediately see that it would be any more efficient.

it would primarily be simpler, because you have a single interface instead of two. As far as I can tell, you'd never mix-n-match the OperatorHandler and the Dialect anyway. It would be more efficient to allow the rust API to specify a static/compile-time operator -> function mapping. It allows the optimizer to see through those calls in the LTO step.

The Dialect represents the complete opcode-to-functionality map, so it really is just an OperatorHandler plus the q and a values. Making it a trait would mean we add a function to get the q and a values from the Dialect instead of just having publicly accessible fields.

I haven't benchmarked it, but I'd be surprised to see the FLookup (it's a [Option<OpFn>; 256]) implementation being slower than the let f = match b[0] { 3 => op_if, ... implementation in your PR. I'd think they both produce a look-up table, so O(1).

I don't think the LTO would be able to do anything without further changes to code here https://github.com/Chia-Network/clvm_rs/blob/main/src/run_program.rs#L299

Making it a trait would mean we add a function to get the q and a values from the Dialect instead of just having publicly accessible fields.

I don't think you have to do it that way. Surely a trait can declare names of constants. These opcodes is known at compile time, so they don't need to be stored in variables. Functions would allow an implementation to keep them as run-time values though, but a chia_rs library wouldn't need that.

I haven't benchmarked it, but I'd be surprised to see the FLookup (it's a [Option; 256]) implementation being slower than the let f = match b[0] { 3 => op_if, ... implementation in your PR. I'd think they both produce a look-up table, so O(1).

I would expect the our operator functions are so slow that they would dwarf the operator lookup, but once we make those faster I wouldn't be surprised this might be next. I would expect the branch prediction benefits of static calls, rather than indirect calls.

I don't see the value in requiring to keep all this state in memory. As far as I can tell, it's more code and complexity.

I don't think the LTO would be able to do anything without further changes to code here https://github.com/Chia-Network/clvm_rs/blob/main/src/run_program.rs#L299

right, that shouldn't be a dynamic interface

src/dialect.rs

arvidn · 2021-08-24T05:43:36Z

src/lib.rs

-mod serialize;
+pub mod reduction;
+pub mod run_program;
+pub mod serialize;


it makes me a bit nervous to expose so much from the library. Is it necessary?

Good question. I don't know the answer to that offhand. The main issue is that we want clvm_rs to be a crate, and use it with chia_rs (and maybe other things). But since there are no users of it yet, it's hard to know what to export. Exposing API to python uses a different mechanism.

I suppose a safer way of doing it is to keep as much private as possible, and when clients need new API, we be explicit and judicious about exposing it. We may end up doing new releases just to expose new API.

I think it's a good idea to expose as little as possible and extend it as we discover new things that need to be public. That gives us an opportunity to think through how we'd like to expose various functionality too.

github-actions · 2021-10-26T11:03:55Z

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

First crack at Dialect.

ca503f6

richardkiss force-pushed the dialect branch 2 times, most recently from 7f312d2 to 738b3b0 Compare August 23, 2021 22:01

Use Dialect where possible.

2fa09f5

richardkiss force-pushed the dialect branch from 738b3b0 to 2fa09f5 Compare August 23, 2021 22:01

arvidn reviewed Aug 24, 2021

View reviewed changes

richardkiss added 4 commits August 24, 2021 16:25

Refactor

0eed276

Make pre_eval non-optional.

8fc7222

Rollback pub.

e3df5c1

Make RunProgramContext private.

15e0c15

github-actions bot added the stale-pr label Oct 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust-based Dialect api. #109

Rust-based Dialect api. #109

richardkiss commented Aug 23, 2021

arvidn left a comment

arvidn Aug 24, 2021

richardkiss Aug 25, 2021

arvidn Aug 26, 2021

arvidn Aug 24, 2021

arvidn Aug 24, 2021

richardkiss Aug 25, 2021

arvidn Aug 26, 2021

arvidn Aug 24, 2021

richardkiss Aug 25, 2021

arvidn Aug 26, 2021

richardkiss Aug 26, 2021

richardkiss Aug 27, 2021

arvidn Aug 27, 2021

arvidn Aug 27, 2021

arvidn Aug 24, 2021

richardkiss Aug 25, 2021

arvidn Aug 26, 2021

github-actions bot commented Oct 26, 2021

Rust-based Dialect api. #109

Are you sure you want to change the base?

Rust-based Dialect api. #109

Conversation

richardkiss commented Aug 23, 2021

arvidn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 26, 2021