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

Sqlite improvements #61

Merged
merged 38 commits into from
Sep 2, 2024
Merged

Sqlite improvements #61

merged 38 commits into from
Sep 2, 2024

Conversation

bhansconnect
Copy link
Collaborator

I think this will be a really cool api improvement that also should give us some perf gains. That said, currently breaks the compiler. Putting the PR up so that other can look at it. Maybe someone else has some ideas of things to mess with to get it compiling. My guess is that tasks + record builders do not play happily together, but the root cause my be something else.

Really want to land this if possible, but might take compiler fixes. I think the impl is pretty cool. Thanks @agu-z for the record bulder help.

@bhansconnect bhansconnect changed the base branch from main to refactor-host July 30, 2024 22:48
@lukewilliamboswell lukewilliamboswell self-assigned this Jul 31, 2024
@bhansconnect bhansconnect changed the base branch from refactor-host to main July 31, 2024 19:27
@bhansconnect
Copy link
Collaborator Author

Ok, re-aimed this at main now that refactor host is merged.

@bhansconnect
Copy link
Collaborator Author

bhansconnect commented Aug 2, 2024

Update here. Have been working with Luke and iterating on both api and usability. So Things are shifting again now. Kinda an interactive review.

@bhansconnect
Copy link
Collaborator Author

@Anton-4, I renamed some files and am getting CI failures now. Probably am missing something obvious, but I can't seem to figure out why it is still pulling in the old name.

@Anton-4
Copy link
Collaborator

Anton-4 commented Aug 3, 2024

I fixed the old InternalSql references, perhaps you forgot to commit a file?

Now, they all get stuck at:

 + roc build --prebuilt-platform --linker=legacy ./examples/sqlite.roc
thread '<unnamed>' panicked at crates/compiler/mono/src/ir.rs:6132:56:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@bhansconnect
Copy link
Collaborator Author

I fixed the old InternalSql references, perhaps you forgot to commit a file?

Yeahs, something must not be matching locally with what I have pushed. I know I updated those file...

  • roc build --prebuilt-platform --linker=legacy ./examples/sqlite.roc
    thread '' panicked at crates/compiler/mono/src/ir.rs:6132:56:
    called Option::unwrap() on a None value
    note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

Yeah, this broke recently when I was trying to make the tags nicer. Have to figure it out still.

@bhansconnect
Copy link
Collaborator Author

bhansconnect commented Aug 3, 2024

For the sql file. Locally, I had InternalSql.roc, but apparently my git isn't case sensitive? I had to full delete the file and re-add it to get git to rename the file to InternalSql.roc.

@Anton-4
Copy link
Collaborator

Anton-4 commented Aug 3, 2024

but apparently my git isn't case sensitive?

Huh, that's unexpected 😄

@lukewilliamboswell
Copy link
Collaborator

$ roc run examples/todos.roc
🔨 Rebuilding platform...
thread 'main' panicked at crates/compiler/gen_llvm/src/llvm/build.rs:5770:19:
Error in alias analysis: error in module ModName("UserApp"), function definition FuncName("(\x00\x00\x00^\x00\x00\x00\x18:;\x90\xb2R\x9d?"): expected type 'union { (union { ((),), ((heap_cell,),), (), ((heap_cell,), union { (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), () }), (), ((),), () },), ((heap_cell, bag<(heap_cell,)>),) }', found type 'union { (union { ((),), ((heap_cell,),), ((heap_cell,), union { (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), (), () }), ((),) },), ((heap_cell, bag<(heap_cell,)>),) }'
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@lukewilliamboswell lukewilliamboswell removed their assignment Aug 23, 2024
@lukewilliamboswell lukewilliamboswell added blocked enhancement New feature or request labels Aug 23, 2024
let alloc_result = heap.alloc_for(stmt);
match alloc_result {
Ok(out) => {
// TODO: only make these constant if they escape init.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do think this TODO needs to be dealt with before we land. Just an FYI

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bhansconnect I think I resolved this in fd317fb. Can you just double check as I wasn't 100% around the logic here. Like are we setting refcount to zero during or after init. I've opted for during as that feels right, these stmts will escape init and therefore we don't want them to be freed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I realized a cooler way to do this. Just pushed it. It also has the advantage that an sqlite statement that is freed during init will actually get freed. Only those still live after init will be promoted to constants!

@bhansconnect
Copy link
Collaborator Author

Ok, I think this is good for final review and merge!

Copy link
Collaborator

@lukewilliamboswell lukewilliamboswell left a comment

Choose a reason for hiding this comment

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

Woo! 🐙

@lukewilliamboswell lukewilliamboswell merged commit a50a088 into main Sep 2, 2024
4 checks passed
@lukewilliamboswell lukewilliamboswell deleted the sqlite-improvements branch September 2, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants