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

Add the update ABI #2137

Merged
merged 8 commits into from
Jan 27, 2025
Merged

Add the update ABI #2137

merged 8 commits into from
Jan 27, 2025

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Jan 17, 2025

Description of Changes

Fixes #2015.

Performance wise, this PR builds upon #2092, where the perf numbers were:

for i in {1..5}; do spacetime call basics update_positions_by_id -s local; done

2025-01-07T23:56:50.151582Z  INFO: : Timing span "update_positions_by_id": 1.516822083s
2025-01-07T23:56:52.097840Z  INFO: : Timing span "update_positions_by_id": 1.451819161s
2025-01-07T23:56:54.108770Z  INFO: : Timing span "update_positions_by_id": 1.547491017s
2025-01-07T23:56:56.071318Z  INFO: : Timing span "update_positions_by_id": 1.50619533s
2025-01-07T23:56:58.155637Z  INFO: : Timing span "update_positions_by_id": 1.569858904s

for i in {1..5}; do spacetime call basics update_positions_by_collect -s local; done

2025-01-07T23:52:44.146575Z  INFO: : Timing span "update_positions_by_collect": 1.10285933s
2025-01-07T23:52:45.743800Z  INFO: : Timing span "update_positions_by_collect": 1.071438136s
2025-01-07T23:52:47.273903Z  INFO: : Timing span "update_positions_by_collect": 1.060757941s
2025-01-07T23:52:48.852988Z  INFO: : Timing span "update_positions_by_collect": 1.089778179s
2025-01-07T23:52:50.448466Z  INFO: : Timing span "update_positions_by_collect": 1.058645907s

Perf numbers on this PR:

for i in {1..5}; do spacetime call basics update_positions_by_id -s local; done

2025-01-17T20:58:06.177181Z  INFO: : Timing span "update_positions_by_id": 961.479454ms
2025-01-17T20:58:07.874284Z  INFO: : Timing span "update_positions_by_id": 1.077129883s
2025-01-17T20:58:09.489971Z  INFO: : Timing span "update_positions_by_id": 1.132045087s
2025-01-17T20:58:11.187569Z  INFO: : Timing span "update_positions_by_id": 1.185711677s
2025-01-17T20:58:13.006517Z  INFO: : Timing span "update_positions_by_id": 1.196531793s

for i in {1..5}; do spacetime call basics update_positions_by_collect -s local; done

2025-01-21T04:14:53.970948Z  INFO: : Timing span "update_positions_by_collect": 664.876291ms
2025-01-21T04:14:55.258220Z  INFO: : Timing span "update_positions_by_collect": 651.551835ms
2025-01-21T04:14:56.600896Z  INFO: : Timing span "update_positions_by_collect": 667.1316ms
2025-01-21T04:14:57.722544Z  INFO: : Timing span "update_positions_by_collect": 638.108509ms
2025-01-21T04:14:58.980541Z  INFO: : Timing span "update_positions_by_collect": 642.46669ms

API and ABI breaking changes

This is additive in term of the syscall ABIs and in terms of the module bindings APIs, there isn't really any change.

Expected complexity level and risk

5, the datastore logic is rather complicated and critical.
However, there are a lot of tests now.

Testing

Lots of new tests are added named fn test_update_* that try to cover the complicated logic and to achieve near full path coverage.

Unlike other parts of our datastore with bugs in terms of create_index transactionality (e.g., btree_scan and iter_by_col_eq), this PR tries to be correct, which unfortunately makes the code all the less elegant.

Reviewer notes

I highly recommend hiding whitespace changes when reviewing.

@Centril Centril force-pushed the centril/update-abi branch from 12fd370 to 059463f Compare January 17, 2025 18:58
Base automatically changed from centril/index-key-by-id to master January 17, 2025 19:08
@Centril Centril force-pushed the centril/update-abi branch 3 times, most recently from 6770f62 to bc2a869 Compare January 21, 2025 04:28
@Centril Centril requested a review from gefjon January 21, 2025 18:29
@Centril Centril marked this pull request as ready for review January 21, 2025 18:29
@bfops bfops added the release-any To be landed in any release window label Jan 21, 2025
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

I am approving the addition of the update_mut_tx function to crates/core/src/db/datastore/traits.rs

@Centril Centril force-pushed the centril/update-abi branch from bc2a869 to f388240 Compare January 22, 2025 12:00
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Looks good, and I really appreciate the tests. Just a few minor questions, then I'll be happy to approve.

crates/core/src/host/instance_env.rs Outdated Show resolved Hide resolved
crates/table/src/table.rs Outdated Show resolved Hide resolved
@Centril Centril mentioned this pull request Jan 27, 2025
@Centril Centril force-pushed the centril/update-abi branch from f388240 to a1aeb6c Compare January 27, 2025 16:10
@bfops
Copy link
Collaborator

bfops commented Jan 27, 2025

The SDK Tests failures should be addressed once clockworklabs/com.clockworklabs.spacetimedbsdk#223 merges.

bfops added a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Jan 27, 2025
## Description of Changes
Remove the `-hotfix*` part of the version in our trunk branch.

It was a (my) mistake to merge a `-hotfix*` version change. Hotfixes
are, by definition, cherry-picked changes that are going to be released
directly to users without merging.

The consequence of merging this change was that the `SDK Tests` CI job
started failing on SpacetimeDB PRs (e.g. see failures on
clockworklabs/SpacetimeDB#2137), because it
checks out this repo, which then tried to use the `-hotifx3` version of
SpacetimeDB. But the `master` branch of SpacetimeDB is at `1.0.0-rc3`
(no hotfix), because the hotfix was correctly released from a branch
without merging in that repo.

Although this PR reverts the version change, we do still have a tag
`v1.0.0-rc3-hotfix3` that we can use to release the hotfix version (by
`git push -f origin v1.0.0-rc3-hotfix3:master`) if/when desired.

## API

 - [ ] This is an API breaking change to the SDK

No

## Requires SpacetimeDB PRs
Should work with `master`.

## Testsuite
*If you would like to run the your SDK changes in this PR against a
specific SpacetimeDB branch, specify that here. This can be a branch
name or a link to a PR.*

SpacetimeDB branch name: master

## Testing
I claim that the CI tests passing with `master` show that this is
correct. The original version change itself passed CI because it was
pointing at a non-`master` branch for testing. I claim it would have
failed if it were tested against SpacetimeDB `master`.

Generally, this might point to a bug in how we've done this CI: We
should probably only allow a PR to merge in this repo if it tests
successfully against SpacetimeDB `master`, even if we want to point it
at other branches to test in the meantime.

Co-authored-by: Zeke Foppa <[email protected]>
@Centril Centril added this pull request to the merge queue Jan 27, 2025
Merged via the queue into master with commit afdc0d6 Jan 27, 2025
13 checks passed
@Centril Centril deleted the centril/update-abi branch January 27, 2025 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

perf: add update syscall that will avoid the delete + insert pair
4 participants