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

[ABI] Remove the special first element of iterator #420

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

RReverser
Copy link
Member

Description of Changes

I'm not sure why we're sending encoded schema as this special first element of iteration.

There is some commented out code that suggests it might have been used in the past, but at least nowadays modules (both Rust and C#) are strongly typed and already know the schema of the tables they asked to iterate over, and ignore the first element returned from the iteration.

As a result, we've been unnecessarily reading schema, encoding it into binary, taking care to allocate it as individual small blob, stored in list of buffers and sent it between Wasm and Rust just for it to be ignored.

Removing it simplifies code and if we ever want to get table schema, we can do that via a dedicated method rather than send it automatically on beginning of each iteration.

This is an ABI breaking change, but likely worth it.

API and ABI

  • This is a breaking change to the module ABI
  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

Expected complexity level and risk

How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

@RReverser

This comment was marked as outdated.

@kulakowski
Copy link
Contributor

Description of Changes

I'm not sure why we're sending encoded schema as this special first element of iteration.

There is some commented out code that suggests it might have been used in the past, but at least nowadays modules (both Rust and C#) are strongly typed and already know the schema of the tables they asked to iterate over, and ignore the first element returned from the iteration.

As a result, we've been unnecessarily reading schema, encoding it into binary, taking care to allocate it as individual small blob, stored in list of buffers and sent it between Wasm and Rust just for it to be ignored.

Removing it simplifies code and if we ever want to get table schema, we can do that via a dedicated method rather than send it automatically on beginning of each iteration.

Yes, we used to expose out of the Rust bindings a non-strongly-typed iteration. I don't know if it was ever actually used, but I think for Chesteron's fence type reasons, someone should speak to why it was ok to remove but to leave the code commented out in the bindings: https://github.com/clockworklabs/SpacetimeDB/blob/master/crates/bindings/src/lib.rs#L272 and so on (and all the commented out code which used that, like the delete_filter etc functions).

@RReverser
Copy link
Member Author

Rebased & fixed conflicts.

I'm not sure why we're sending encoded schema as this special first element of iteration.

There is some commented out code that suggests it might have been used in the past, but at least nowadays modules (both Rust and C#) are strongly typed and already know the schema of the tables they asked to iterate over, and ignore the first element returned from the iteration.

As a result, we've been unnecessarily reading schema, encoding it into binary, taking care to allocate it as individual small blob, stored in list of buffers and sent it between Wasm and Rust just for it to be ignored.

Removing it simplifies code and if we ever want to get table schema, we can do that via a dedicated method rather than send it automatically on beginning of each iteration.

This is an ABI breaking change, but likely worth it.
@RReverser RReverser force-pushed the remove-special-first-elem branch from a7f3604 to 1dbc958 Compare October 16, 2023 18:30
@coolreader18
Copy link
Collaborator

IIRC it was like this because fn __iter__ took an arbitrary table id without any type information, and it was designed to return an iterator over AlgebraicValue without needing any generics or TableType or anything. (It also had FromSats and ToSats that took/returned an AlgebraicValue directly, rather than the Serialize/Deserialize traits we have now, if that makes that design make more sense.) When I made the changes away, I considered removing the now-vestigial row type bit, but didn't because I was unsure if maybe there was a vision for allowing more dynamic db operations, but clearly that's not come out to much. Feel free to remove it, it's totally vestigial at this point.

@cloutiertyler
Copy link
Contributor

cloutiertyler commented Oct 16, 2023

I just synced with Noa as well. TL;DR this is approx how it used to work in psuedo code:

let algebraic_type = AlgebraicType::decode_from_bytes(algebraic_type_bytes);
let algebraic_value = AlgebraicValue::decode_from_bytes(bytes, algebraic_type);
let my_table: MyTable = MyTable::from_algebraic_value(algebraic_value);

So we actually used to use both the static and dynamic type information. @coolreader18 switched it to be:

let my_table: MyTable = MyTable::from_bytes(bytes);

but left the dynamically sent AlgebraicType bytes in there in case it was used eventually. Ultimately if we were to have a dynamic API though, you probably wouldn't want it to be mandatory with every iter call, you'd want to have a separate API/ABI sys call for getting the AlgebraicType of a table. So this is good to be merged and Chesterson can rest easy.

@RReverser
Copy link
Member Author

So this is good to be merged and Chesterson can rest easy.

Glad to hear :) Can one of you set an approval tick please?

@RReverser RReverser merged commit 469dff6 into master Oct 16, 2023
5 checks passed
@RReverser RReverser deleted the remove-special-first-elem branch October 16, 2023 21:45
cloutiertyler added a commit that referenced this pull request Oct 18, 2023
jdetter pushed a commit that referenced this pull request Oct 31, 2023
@jdetter jdetter mentioned this pull request Oct 31, 2023
4 tasks
@kulakowski kulakowski added the abi-break A PR that makes an ABI breaking change label Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break A PR that makes an ABI breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants