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

Diesel Performance Fixes, Batching Improvements, New Allocator #262

Merged
merged 16 commits into from
Jan 7, 2025

Conversation

jr1221
Copy link
Contributor

@jr1221 jr1221 commented Dec 21, 2024

Changes

  • Switch to diesel-async, to better use the tokio runtime to organize threads
    Rationale: Performance of blocking threads decreases with more threads in use, affecting large chunk batching and the [Scylla] - Investigate a new CSV handler #243 code
  • Switch to jemalloc, as extreme memory use was being observed with [Scylla] - Investigate a new CSV handler #243
    Rationale: The system allocator was not freeing the memory used by the batching tasks effectively, resulting in excessive memory usage. It takes now approximately 1-2gb of RAM to upload an hour of data, and that RAM is released back when the upload is complete. Previously it was 5-10gb, and the memory was not freed.
  • Modify the values field of the DATA table to be NOT NULL, modify the values field to be REAL instead of DOUBLE. PRECISION.
    Rationale (found in the docs for DataInsert): This aligns closer to the actual data we recieve, which is not null 4 byte floating point integers. This allows less re-allocation of data when converting our recieved protobuf to the data the database is looking for, improving performance.
  • Redo chunking logic.
    Rationale: Before, chunks of data to upload were split into a few even chunks and one chunk of only a few points. The new algorithm better evens out the chunks. It still could use improvement, and more investigation into the libpq instruction limit could be warrented.

Notes

Jemalloc may fail to build on aarch64, hence the CI running, so we should pay attention to any unsoundness or stability issues.

Test Cases

  • All functionality normal, perhaps a little faster.

To Do

Any remaining things that need to get done

  • Ensure CI works

Checklist

It can be helpful to check the Checks and Files changed tabs.
Please review the contributor guide and reach out to your Tech Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.

  • All commits are tagged with the ticket number
  • No linting errors / newline at end of file warnings
  • All code follows repository-configured prettier formatting
  • No merge conflicts
  • All checks passing
  • Screenshots of UI changes (see Screenshots section)
  • Remove any non-applicable sections of this template
  • Assign the PR to yourself
  • No package-lock.json changes (unless dependencies have changed)
  • Request reviewers & ping on Slack
  • PR is linked to the ticket (fill in the closes line below)

Closes #244
Closes #184

@jr1221 jr1221 self-assigned this Dec 21, 2024
@jr1221 jr1221 requested a review from bracyw January 5, 2025 21:37
tokio::task::spawn_blocking(move || {
DbHandler::batch_upload(owned, pool)});
let msg_len = msgs.len();
let chunk_size = msg_len / ((msg_len / 8190) + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you now dividing by 8190 instead of 16380, something special about dividing max params by 8 instead of 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ya forgot to investigate that. The switch to diesel async doubled the number of instructions per insert. I'll do some investigating there. It's a very annoying limit.

@@ -81,17 +110,16 @@ impl DbHandler {
// libpq has max 65535 params, therefore batch
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually never understood this fully. why is the batching logic needed for diesel... is it because it let's you try and insert more data then libpq can handle. Does prisma just manage this itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess prisma splits the queries as needed. I doubt they work around libpq as it's kinda the best way to communicate with postgres. It's a very annoying limit but kinda inherent to postgres.

Copy link
Contributor

@bracyw bracyw left a comment

Choose a reason for hiding this comment

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

Integration tests fail sometimes when you either have the database disabled in docker or haven't set it up yet. I think just adding a sleep like your github tests. For now we can probably just restart or remove the database if it is already a docker container but disabled.... I can push changes for this if you want.

everything compiles tests and runs fine on mac m1.

@jr1221
Copy link
Contributor Author

jr1221 commented Jan 6, 2025

Ya can unadd the fix to integration tests? Thanks.

@bracyw bracyw self-requested a review January 7, 2025 03:08
Copy link
Contributor

@bracyw bracyw left a comment

Choose a reason for hiding this comment

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

LGTM

@jr1221 jr1221 merged commit 86cf64c into develop Jan 7, 2025
4 checks passed
@jr1221 jr1221 deleted the 244-batch-diesel-perf branch January 7, 2025 03:13
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.

[Scylla] - Fix batching logic [Scylla] - Investigate performance improvements
2 participants