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

Inline ops that read settings #2

Merged
merged 2 commits into from
Apr 20, 2023

Conversation

miguelff
Copy link

Creating the patch first against the branch we use in prisma-engines.

@miguelff miguelff force-pushed the fetch-settings-in-one-query branch from 5b62283 to 41338e7 Compare April 19, 2023 14:47
@miguelff miguelff marked this pull request as ready for review April 19, 2023 14:57
@miguelff
Copy link
Author

Screenshot 2023-04-19 at 17 03 04

Measures taken on 100 coldstart connections:

Before patch:
Average: 222.01605959
Standard deviation: 20.28249939282523

After patch:

Average: 152.8317912
Standard deviation: 22.382571070107467

Inline queries makes connection run ~30% faster.

src/conn/mod.rs Outdated
Some(value)
// set max_allowed_packet
let max_allowed_packet = if read_max_allowed_packet {
settings.as_ref().map(|s| s.1).unwrap()
Copy link

Choose a reason for hiding this comment

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

Nitpick: you could get the result as a Row and then use row::get("column_name"). Might increase visibility, nothing blocking anyway.

Copy link
Author

Choose a reason for hiding this comment

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

Good feedback, addressing.

Copy link

@Weakky Weakky left a comment

Choose a reason for hiding this comment

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

Nice! 👍

@miguelff
Copy link
Author

Cool, did a pass to the test suite using the same configuration defined in azure-pipeliens.yml. With MariaDB 10.5. Tests passed. When removing the line that reads the settings a few tests break. Tests indeed cover reading the settings.

@miguelff miguelff merged commit dad187b into vendored-openssl Apr 20, 2023
@miguelff miguelff deleted the fetch-settings-in-one-query branch April 20, 2023 07:35
miguelff pushed a commit that referenced this pull request Apr 20, 2023
Measures taken on 100 coldstart connections to a remote RDS mysql 5.7 in us-east-1, accessed from Spain. Units are ms.

Before patch:

Average: 222.01605959
Standard deviation: 20.28249939282523

After patch:

Average: 152.8317912
Standard deviation: 22.382571070107467

Inlining queries make connections that require reading server settings run ~30% faster.
miguelff added a commit to prisma/prisma-engines that referenced this pull request Apr 20, 2023
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.

2 participants