Simplify/deabstract module_host_actor and wasmer_module #417
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This will conflict with #267 |
|
Not to jump the gun, but I took a peek and think this is going to be helpful to people clicking around in and reading this code. |
|
Not saying this is bad, but it will make @mamcx’s life harder to get #267 over the line. Also as a heads up: with the changes in #267 much of the init/update code becomes obsolete (for one because we can no longer update index changes). Please let me know when this becomes ready, I think I’m mostly the one familiar with that code. |
|
More than happy to wait on #267 to be merged first. |
coolreader18
force-pushed
the
noa/deabstract
branch
from
5c319da to
595acf7
Compare
4 tasks
4 tasks
coolreader18
force-pushed
the
noa/deabstract
branch
from
595acf7 to
dd11a77
Compare
|
What's the status with #267 re:being split into multiple parts? I can't tell whether the chunks that are relevant to wasmer_module are in master already or not. |
coolreader18
force-pushed
the
noa/deabstract
branch
from
dd11a77 to
94b8f2d
Compare
|
benchmarks please |
|
Sorry, you don't have permission to run benchmarks. |
|
lol. well |
|
I guess that confirms that works @kurtismullins |
|
benchmarks please |
|
Sorry, you don't have permission to run benchmarks. |
coolreader18
force-pushed
the
noa/deabstract
branch
from
94b8f2d to
bf10a1c
Compare
|
benchmarks please |
Is still waiting for approving, and it will take some time because was split in several other PRs... |
|
Alright, then let’s merge this? |
|
@coolreader18 Maybe you could add this from #386: https://github.com/clockworklabs/SpacetimeDB/pull/386/files#diff-840af797a96dc965f1b75269b1c343356ecb6a0b4e28b84c611a0db514e9d3faR113-R127 ? I think it is still better meanwhile to create multi-column indexes than to ignore them. |
|
Maybe as a separate PR based on this one 😬 ? unless I missed that when I was rebasing and it's in master already. Just cause it's kinda its own feature rather than a refactor like this. |
|
benchmarks please |
Benchmark resultsBenchmark ReportLegend:
All throughputs are single-threaded. Empty transaction
Single-row insertions
Multi-row insertions
Full table iterate
Find unique key
Filter
Serialize
Module: invoke with large arguments
Module: print bulk
Remaining benchmarks
|
|
cool it works :)))) |
kulakowski
reviewed
Oct 27, 2023
There was a problem hiding this comment.
Ok I think this is good and ready. It does a bit of unrelated cleanups but I like them all.
I'm not hitting approve just to make sure that we are all squared up on which PR lands before whichever other PR, but I'll hit it as as soon as someone acknowledges that the plan is ready.
| @@ -370,11 +369,7 @@ impl RelationalDB { | |||
|
|
|||
| /// Roll back transaction `tx` if `res` is `Err`, otherwise return it | |||
| /// alongside the `Ok` value. | |||
| #[tracing::instrument(skip_all)] | |||
There was a problem hiding this comment.
I'd keep the instrument bit.
There was a problem hiding this comment.
This is the only thing to fix before landing, then, I think.
There was a problem hiding this comment.
IMO it's really unnecessary - it's a utility function that we have no need to track and only serves to clutter up logs/graphs. If you feel strongly about it I can add it back, but I really see no value in it.
There was a problem hiding this comment.
It's not that I do or don't see the value in it. It's that it is unrelated to the stated point of the PR.
| @@ -292,6 +293,73 @@ pub struct TableDef { | |||
| pub(crate) table_access: StAccess, | |||
| } | |||
|
|
|||
| impl TableDef { | |||
There was a problem hiding this comment.
I don't love this code being here. But I don't think there's a great place to put it when we are in this state of defining a pile of concrete types, and traits (that aren't really used, relationaldb directly makes a Locking or whatever). Curious how you feel about it.
There was a problem hiding this comment.
Yeah, I think there's better ways to go about this, but ideally that would come with a general reorganizing of all these types.
There was a problem hiding this comment.
I also don't love this code being here FWIW
| Database(#[from] DBError), | ||
| } | ||
|
|
||
| pub fn update_database( |
There was a problem hiding this comment.
I didn't scrutinize this code, am trusting that it's functionally unchanged.
kulakowski
approved these changes
Oct 27, 2023
4 tasks
|
Merged. @joshua-spacetime if you'd like help fixing #486 to work with this I'd be happy to help |
|
Thanks @coolreader18. The conflicts weren't too bad. I was able to get them all resolved. But feel free to take a look just in case. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Changes
API and ABI
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.