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

Bench for SQL scan / where #370

Merged
merged 3 commits into from
Oct 6, 2023
Merged

Bench for SQL scan / where #370

merged 3 commits into from
Oct 6, 2023

Conversation

mamcx
Copy link
Contributor

@mamcx mamcx commented Oct 5, 2023

Description of Changes

  • Bench for SQL queries: SELECT * FROM table / SELECT * FROM table WHERE pk = value
  • Instrument calls for tracy
  • Add tracy support in run_standalone_temp.sh

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

@mamcx mamcx requested a review from joshua-spacetime October 5, 2023 19:54
Copy link
Collaborator

@joshua-spacetime joshua-spacetime left a comment

Choose a reason for hiding this comment

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

LGTM!

@mamcx mamcx merged commit 05956d6 into master Oct 6, 2023
@kazimuth
Copy link
Contributor

kazimuth commented Oct 10, 2023

This PR broke the module benchmarks, FWIW. There is also a way to structure it that wouldn't have caused the issue. I would have appreciated if you had waited for my review.

@mamcx
Copy link
Contributor Author

mamcx commented Oct 10, 2023

So, what should I do instead?

@kazimuth
Copy link
Contributor

kazimuth commented Oct 10, 2023

Rather than adding trait methods to BenchDatabase, it would be best to add a separate file spacetime_sql.rs with a new struct SpacetimeSQL : BenchDatabase that measures database access via the SQL API. This new file could use TableSchema for TableId.

This means that each generic benchmark implementation is measuring a separate access path to the database. Times of the SQL api will be clearly separate from times of reducer calls.

The specific thing that broke here is that a PascalCase name has been used instead of a snake_case due to the refactoring in this branch, here: https://github.com/clockworklabs/SpacetimeDB/pull/370/files#diff-6179b2533c85dd1dc6d18d56e1bff1a01fb6c4dfcd2861a08d7ea565ad1f02fbR192

I'm working on a branch that reverts this, so that benchmarks will run on master. Then I'll start a new PR to add the spacetime-SQL benches, we can collab on that :)

(We'll need to have no-ops for insertion benchmarks there, in some way. I guess we'll need to add an insertion_supported() -> bool method to BenchDatabase.)

kazimuth added a commit that referenced this pull request Oct 10, 2023
kazimuth added a commit that referenced this pull request Oct 10, 2023
kazimuth added a commit that referenced this pull request Oct 12, 2023
* Reverts benchmarks portion of "Bench for SQL scan / where (#370)"
This partially reverts commit 05956d6.

* Get benchmarks runner to remember what it has installed, hopefully

* Fix build error
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.

3 participants