websocket: send/receive reducer & table ids instead of names #1883

Centril · 2024-10-21T21:45:51Z

Description of Changes

Receive and send reducer ids and table ids as opposed to names in the SDK.

TODO:

Host implementation of the above.
Rust SDK implement of the above.
C# SDK implementation of the above, Companion to SpacetimeDB#1883 (ids-no-names) com.clockworklabs.spacetimedbsdk#178
TS SDK implementation of the above

Fixes https://github.com/clockworklabs/SpacetimeDBPrivate/issues/1091.

API and ABI breaking changes

Yes.

gefjon

Please update the PR description to match the template.
I would like to see a Rust SDK test added which verifies that it's possible to construct a connection and then immediately invoke a reducer, without first waiting for the on-connect callback. I would expect this to queue the reducer call until the handshake message is received. The code appears to do this correctly, but I want a test to make sure we don't regress it in the future.

gefjon · 2024-11-01T16:00:21Z

crates/cli/src/subcommands/generate/mod.rs

-    fn generate_table(&self, module: &ModuleDef, namespace: &str, tbl: &TableDef) -> String;
+    fn generate_table(&self, idx: u32, module: &ModuleDef, namespace: &str, tbl: &TableDef) -> String;
    fn generate_type(&self, module: &ModuleDef, namespace: &str, typ: &TypeDef) -> String;
-    fn generate_reducer(&self, module: &ModuleDef, namespace: &str, reducer: &ReducerDef) -> String;
+    fn generate_reducer(&self, idx: u32, module: &ModuleDef, namespace: &str, reducer: &ReducerDef) -> String;


Why this change to add indices to the client codegen? I believe, but would like it confirmed, that these are not table IDs which could be sent over a WebSocket; rather, they appear to be an unrelated optimization to the SDK internals.

If that is the case, I would strongly prefer this be broken out into a separate PR, and frankly I would see very little reason to approve such a PR without measurements showing that HashMap lookups of table and reducer name strings are a significant performance overhead in the SDKs. I would also question why we'+ prefer to introduce a new ID to this interface, rather than a change purely internal to the codegen, possibly involving either generating these indices within the codegen, or possibly involving a perfect hash function.

If that is not the case, and this change is necessary or useful towards the actual goal of this PR, namely transmitting integer IDs rather than name strings over the WebSocket API, then I would like to see documentation on this trait's methods describing what these indices mean, where they come from and how they're used. I would also like the PR description amended to describe this change.

Why this change to add indices to the client codegen? I believe, but would like it confirmed, that these are not table IDs which could be sent over a WebSocket; rather, they appear to be an unrelated optimization to the SDK internals.

Indeed, these are indices into the 2 arrays, one for reducer names and one for reducer ids and the same for table names <-> table ids. This seemed like the most efficient way to represent things, avoiding hash maps in many cases in favor of just Vec<_>s.

If that is the case, I would strongly prefer this be broken out into a separate PR, [...]

Sure, I can do that, but then it will likely miss the train...

[...] and frankly I would see very little reason to approve such a PR without measurements showing that HashMap lookups of table and reducer name strings are a significant performance overhead in the SDKs. I would also question why we'+ prefer to introduce a new ID to this interface, rather than a change purely internal to the codegen, possibly involving either generating these indices within the codegen, or possibly involving a perfect hash function.

Why it would need to be a significant perf overhead. It seems clear to me that it is a perf win and more predictable to identity hash an u32 and using it as an index into a vector, rather than hashing strings. I don't understand what you mean regarding a change internal to the codegen. Whatever we do, we have to keep mappings K -> ID and sometimes ID -> K in the incoming-message-loop and DbContextImpl, as the values of ID are determined at handshake. K could then be &'static str or an index into the list of reducers/tables. This PR opted for the latter for efficiency reasons.

If that is not the case, and this change is necessary or useful towards the actual goal of this PR, namely transmitting integer IDs rather than name strings over the WebSocket API, then I would like to see documentation on this trait's methods describing what these indices mean, where they come from and how they're used. I would also like the PR description amended to describe this change.

Sounds good, I will definitely add that documentation/amend the PR description.

I do not dispute that this is obviously better for runtime performance, but there are non-technical costs to making changes like this. For example:

I have a stack of outstanding PRs, ending in an actual P1 ticket with actual performance and usability implications for the SDK, which will significantly conflict with this change.

Reviewing this PR was made artificially and unnecessarily more difficult because it included this change, and I had to sort through which parts of the diff related to the stated goal of the PR versus which parts related to this change.

Introducing a third identifier for database objects, in addition to the string names and the runtime IDs, adds significant potential for confusion. It's already not great that tables have both names and IDs, with different semantics, but at least the names are of an obviously distinct type from the IDs, so there's less risk of confusing the two. I believe there to be a very real cost in code complexity and maintenance to adding another integer identifier for database objects which is not interchangeable with IDs.

There is the additional cost that making a larger change may introduce more bugs. As the person who has to deal with all the bugs and their downstream ramifications, I can tell you that the cost is very very high. We just dealt with it most recently with the change to u256 for Identity. The nature of these things is that it always feels like it will probably be fine, but it rarely is and then we pay the price downstream.

I agree with @gefjon, please separate the PR. I don't think this one can go in, but we'll get it in eventually. Either that or we can just have this be a different version of the API and maintain both and eventually deprecate the one Lightfox is using currently.

crates/cli/src/subcommands/generate/mod.rs

gefjon · 2024-11-01T16:07:21Z

crates/core/src/db/datastore/locking_tx_datastore/committed_state.rs

-                let table_name = &*table.get_schema().table_name;
-
                if !deletes.is_empty() {
+                    let table_name = &table.get_schema().table_name;


Is this related to the PR, or just a drive-by change? Is it actually an optimization, i.e. is get_schema expensive?

With the caveat that I haven't spent time on this PR in a few days, this does look like a drive-by change, but also trivial code motion... This is a micro optimization, shaving off a hash map lookup, that LLVM is unlikely to elide.. Mostly though, I think I mostly just wanted to move it closer to the usage, for better readability.

gefjon · 2024-11-01T16:07:30Z

crates/core/src/db/datastore/locking_tx_datastore/committed_state.rs

-            let table_name = &*commit_table.get_schema().table_name;
-
            if !inserts.is_empty() {
+                let table_name = &commit_table.get_schema().table_name;


Same questions.

(see #1883 (comment))

gefjon · 2024-11-01T16:07:50Z

crates/core/src/db/datastore/locking_tx_datastore/datastore.rs

@@ -899,7 +899,6 @@ impl<F: FnMut(u64)> spacetimedb_commitlog::payload::txdata::Visitor for ReplayVi
        reader: &mut R,
    ) -> std::result::Result<Self::Row, Self::Error> {
        let schema = self.committed_state.schema_for_table(table_id)?;
-        // TODO: avoid clone


Why is this comment removed?

It's an Arc now, so cloning is cheap and so we don't need to care anymore.

gefjon · 2024-11-01T16:15:42Z

crates/cli/src/subcommands/generate/typescript.rs

@@ -82,7 +82,7 @@ Requested namespace: {namespace}",
        output.into_inner()
    }

-    fn generate_table(&self, module: &ModuleDef, namespace: &str, table: &TableDef) -> String {
+    fn generate_table(&self, _idx: u32, module: &ModuleDef, namespace: &str, table: &TableDef) -> String {


Why does TypeScript not need or use these indices?

This bit of the PR was not fully done; TS should also be using the indices. I'll get that done today.

gefjon · 2024-11-01T16:16:59Z

crates/core/src/error.rs

-    UnknownField { field: String, tables: Vec<Box<str>> },
+    UnknownField { field: String, tables: Vec<Arc<str>> },
    #[error("Unknown field name: `{field}` not found in the table(s): `{tables:?}`")]
-    UnknownFieldName { field: FieldName, tables: Vec<Box<str>> },
+    UnknownFieldName { field: FieldName, tables: Vec<Arc<str>> },
    #[error("Field(s): `{fields:?}` not found in the table(s): `{tables:?}`")]
-    UnknownFields { fields: Vec<String>, tables: Vec<Box<str>> },
+    UnknownFields { fields: Vec<String>, tables: Vec<Arc<str>> },


Are these (and similar changes from Box to Arc in this PR) related to the goal of the PR? Could they be split into a separate PR?

I think I decided to do this at the point I remembered that we wanted to keep the names in the JSON format but not in the BSATN format. This meant that we would have had to, in most cases, clone e.g., table_name: Box<str> only to throw them away at the end. That seemed wasteful, so I decided to make these into Arcs to avoid the actual unnecessary heap allocations. This could be split into a separate PR, but that would make it harder to merge this in time though.

gefjon · 2024-11-01T16:19:59Z

crates/sdk/src/callbacks.rs

    /// Maps table name to a set of callbacks.
-    table_callbacks: HashMap<&'static str, TableCallbacks<M>>,
+    table_callbacks: IntMap<u32, TableCallbacks<M>>,


Comment is no longer correct. It's also not clear what the actual key is, now - is it the table ID received during handshake, or the table index computed at codegen time?

bfops

LGTM on the CLI changes that I'm a codeowner for - I did not review crates/cli/src/subcommands/generate/mod.rs because that diff is more about "code generation" than about "CLI".

Centril · 2024-11-04T18:12:35Z

Please update the PR description to match the template.

👍

I would like to see a Rust SDK test added which verifies that it's possible to construct a connection and then immediately invoke a reducer, without first waiting for the on-connect callback. I would expect this to queue the reducer call until the handshake message is received. The code appears to do this correctly, but I want a test to make sure we don't regress it in the future.

This is exercised by the test exec_caller_always_notified which failed before I adjusted the SDK to first always process the handshake.

Centril requested review from bfops, jdetter, cloutiertyler and gefjon as code owners October 21, 2024 21:45

bfops added the release-rc1 label Oct 21, 2024

Centril force-pushed the centril/websocket-light branch 2 times, most recently from c659ca4 to 7e91c80 Compare October 22, 2024 20:17

Centril force-pushed the centril/websocket-use-ids branch 4 times, most recently from 13053e0 to 9ed5071 Compare October 23, 2024 02:02

Centril force-pushed the centril/websocket-light branch from 18f4e87 to b6689e9 Compare October 23, 2024 06:56

Centril force-pushed the centril/websocket-use-ids branch from a9e63d5 to fd40008 Compare October 23, 2024 07:02

Centril force-pushed the centril/websocket-light branch from b6689e9 to 33e815c Compare October 23, 2024 07:16

Centril force-pushed the centril/websocket-use-ids branch from fd40008 to 73488dc Compare October 23, 2024 07:57

Centril mentioned this pull request Oct 23, 2024

Companion to SpacetimeDB#1883 (ids-no-names) clockworklabs/com.clockworklabs.spacetimedbsdk#178

Open

Centril force-pushed the centril/websocket-use-ids branch from 3c60243 to 10c465f Compare October 23, 2024 14:50

Centril force-pushed the centril/websocket-light branch from eafc64b to 9c85842 Compare October 23, 2024 15:04

Centril force-pushed the centril/websocket-use-ids branch from 10c465f to e6c7062 Compare October 23, 2024 15:04

Centril mentioned this pull request Oct 23, 2024

Companion to SpacetimeDB#1940 clockworklabs/spacetimedb-typescript-sdk#118

Open

bfops added release-rc1-nice-to-have api-break and removed release-rc1 labels Oct 24, 2024

Centril force-pushed the centril/websocket-light branch from 9c85842 to 01d3cd9 Compare October 30, 2024 12:50

Centril force-pushed the centril/websocket-use-ids branch from b224fd6 to 6cc4313 Compare October 30, 2024 13:50

gefjon requested changes Nov 1, 2024

View reviewed changes

Base automatically changed from centril/websocket-light to master November 4, 2024 17:19

bfops force-pushed the centril/websocket-use-ids branch from 0ff7391 to 7915b10 Compare November 4, 2024 17:37

bfops approved these changes Nov 4, 2024

View reviewed changes

Centril added 11 commits November 5, 2024 14:20

websocket: send/receive reducer & table ids instead of names

4783706

regenerate snaps & sdk/tests/*/src/module_bindings

b1dab7e

fix standalone_integration_test

bf20b11

implement ids-no-names in c# sdk codegen

2997284

fix rebase fallout

da03bf3

fix 'Unit StdbNone' syntax error

4f796f6

bless insta snapshots

ec75753

fix standalone_integration_test more

7efa5ea

3 more reducer files generated, so 3 more namespaces

20954af

fix standalone_integration_test yet again

6fc632b

fix rebase fallout + use Arc<str> more for reducers

42b315b

Centril force-pushed the centril/websocket-use-ids branch from f235f41 to 42b315b Compare November 5, 2024 13:21

Centril mentioned this pull request Nov 5, 2024

websocket: send/receive reducer & table ids instead of names (take 2) #1940

Open

jdetter removed their request for review November 8, 2024 18:11

bfops removed the api-break label Nov 11, 2024

bfops linked an issue Nov 11, 2024 that may be closed by this pull request

Websocket API: Establish reducer/table ids <-> names during handshake (IdentityConnected) #1796

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

websocket: send/receive reducer & table ids instead of names #1883

websocket: send/receive reducer & table ids instead of names #1883

Centril commented Oct 21, 2024 •

edited

Loading

gefjon left a comment

gefjon Nov 1, 2024

Centril Nov 4, 2024

gefjon Nov 4, 2024

cloutiertyler Nov 4, 2024

gefjon Nov 1, 2024

Centril Nov 4, 2024

gefjon Nov 1, 2024

Centril Nov 4, 2024

gefjon Nov 1, 2024

Centril Nov 4, 2024

gefjon Nov 1, 2024

Centril Nov 4, 2024 •

edited

Loading

gefjon Nov 1, 2024

Centril Nov 4, 2024

gefjon Nov 1, 2024

bfops left a comment •

edited

Loading

Centril commented Nov 4, 2024 •

edited

Loading

websocket: send/receive reducer & table ids instead of names #1883

Are you sure you want to change the base?

websocket: send/receive reducer & table ids instead of names #1883

Conversation

Centril commented Oct 21, 2024 • edited Loading

Description of Changes

API and ABI breaking changes

gefjon 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

Centril Nov 4, 2024 • edited Loading

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

bfops left a comment • edited Loading

Choose a reason for hiding this comment

Centril commented Nov 4, 2024 • edited Loading

Centril commented Oct 21, 2024 •

edited

Loading

Centril Nov 4, 2024 •

edited

Loading

bfops left a comment •

edited

Loading

Centril commented Nov 4, 2024 •

edited

Loading