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

Performance regression: UNION in v19 #15466

Closed
systay opened this issue Mar 13, 2024 · 2 comments
Closed

Performance regression: UNION in v19 #15466

systay opened this issue Mar 13, 2024 · 2 comments

Comments

@systay
Copy link
Collaborator

systay commented Mar 13, 2024

In a recent change to the UNION engine primitive, we added logic to make sure the output types of the union columns are correct.

Unfortunately, this made our UNION code much, much slower than before.

The end-to-end test TestUnionAll, if we run it many times, shows the degradation.

On v18 those 10000 iterations ran in a bit over 6 seconds:

--- PASS: TestUnionAll (6.72s)
    --- PASS: TestUnionAll/olap (6.70s)

On v19 though with the same 10000 iterations:

--- PASS: TestUnionAll (210.99s)
    --- PASS: TestUnionAll/olap (210.97s)
@systay
Copy link
Collaborator Author

systay commented Mar 13, 2024

so, checking out the changes, it looks like we're waiting around to get all the input types before we figure out how to coerce the values properly. but really, we only need to do this if we're not sure about the input types from the get-go. i'm thinking we can speed things up a bit:

  1. if we've already figured out the types during the planning stage, we shouldn't have to wait on any inputs and just apply coercion where it's actually needed. right now, we're doing the type calculus and just coercing everything across the board, even though the type is already correct.
  2. once we run the query for the first time, we've got all the type info we need. so, for any subsequent runs using the same plan, we shouldn't have to redo the type calculations.

for a fix, how about:

  1. pull the type calculations and coercion logic out of the Concatenate primitive, so we can be more selective about when to use it.
  2. once we have the full type info, tailor a plan that only coerces what's necessary, without holding up the works waiting for input fields.
  3. if we're missing type info initially, tweak the plan after the first run, based on the types we see. ideally, after this first run, the plan should be as efficient as the one where we had all the type info upfront. right now, there's no interaction between the planning and execution phases, but we could really use some back-and-forth here.

@harshit-gangal
Copy link
Member

TL;DR: There is no performance regression.

Steps Taken to Understand the Issue:

  1. Checked the commit mentioned in the issue, moved to the previous commit, and started making changes. There was no performance regression with the new changes, but this wasn't sufficient as the code had evolved.

  2. Examined the new PR with a simplified, nearly lockless implementation of concatenate. It still showed performance regression. Tried further optimizations but couldn't improve performance. Spent time analyzing the profiler to detect any anomalies, but the flame graphs revealed nothing unusual.

  3. Switched to the main code and made concatenate a passthrough, but there was still a performance regression. At that point, I realized the problem stemmed from another commit, unrelated to the concatenate engine.

  4. Performed git bisect and found the problematic commit: 74278f1. The changes affected how we test the output of statements by comparing fields. These were changes in the test comparison code, which is why they weren't visible in the flame graphs.

  5. Made changes to the test code and ran the tests on release-19.0 and release-20.0, and voila, no performance regression.

Conclusion:
Going forward, we should validate performance regression with direct vtgate execution and not through the mcmp library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants