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

Less copying and a correctness fix for dotProduct #60928

Merged
merged 15 commits into from Mar 20, 2024

Conversation

rschu1ze
Copy link
Member

@rschu1ze rschu1ze commented Mar 6, 2024

This is follow-up to #60202.

There were two problems:

Reviewer note: This PR includes commits

  • 2dc1721 Only refactorings/renamings for more consistency between related code
  • c390974 Replaces the previous "shotgun cast" approach (which calls convertToFullColumn() / memcpy in the worst case 10 x 10 = 100 times) by a single call to convertToFullColumn()
  • a69bcc2 More refactorings
  • 245ea0c Implements a shortcut for const / non-const arguments which gets rid of the copy-ing for the const argument. Also fixes the correctness bug and extends the functional and performance tests.

Runtime with const/non-const arguments of array cardinality 150 on a table with 50 million rows (a typical scenario for vector search) goes down from 96.9 sec to 0.32 sec.

Changelog category (leave one):

  • Performance Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Optimized function dotProduct to omit unnecessary and expensive memory copies.

@rschu1ze rschu1ze changed the title Dot product memcpy Less memory copying in dotProduct Mar 6, 2024
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-performance Pull request with some performance improvements label Mar 6, 2024
@robot-ch-test-poll1
Copy link
Contributor

robot-ch-test-poll1 commented Mar 6, 2024

This is an automated comment for commit 2c0612c with description of existing statuses. It's updated for the latest CI running

⏳ Click here to open a full report in a separate page

Check nameDescriptionStatus
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR⏳ pending
Successful checks
Check nameDescriptionStatus
Docs checkBuilds and tests the documentation✅ success
PR CheckThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success

@yariks5s yariks5s self-assigned this Mar 6, 2024
@rschu1ze rschu1ze changed the title Less memory copying in dotProduct Less copying and a correctness fix for dotProduct Mar 6, 2024
@rschu1ze rschu1ze marked this pull request as ready for review March 6, 2024 17:07
@alexey-milovidov alexey-milovidov added pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-cloud labels Mar 6, 2024
Comment on lines 249 to 259
if (typeid_cast<const ColumnConst *>(col_x.get()))
{
return executeWithLeftArgConst<ResultType, LeftType, RightType>(col_x, col_y, input_rows_count);
}
else if (typeid_cast<const ColumnConst *>(col_y.get()))
{
return executeWithLeftArgConst<ResultType, RightType, LeftType>(col_y, col_x, input_rows_count);
}

col_x = col_x->convertToFullColumnIfConst();
col_y = col_y->convertToFullColumnIfConst();
Copy link
Member

Choose a reason for hiding this comment

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

We already know here that col_x and col_y are not constant, is there any purpose to do convertToFullColumnIfConst?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can remove it. Interestingly, even if col_x and col_y are not ColumnConst (i.e. they are ColumnArray), calling ColumnArray::convertToFullColumnIfConst() does something:

ColumnPtr ColumnArray::convertToFullColumnIfConst() const
{
    /// It is possible to have an array with constant data and non-constant offsets.
    /// Example is the result of expression: replicate('hello', [1])
    return ColumnArray::create(data->convertToFullColumnIfConst(), offsets);
}

I checked the replicate and arrayWithConstant (the only two SQL functions which look like they could create an array with const data), but they they both create non-const data. It looks to me as if ColumnArray::convertToFullColumnIfConst() can be removed. I might try this in a separate PR.

@rschu1ze
Copy link
Member Author

rschu1ze commented Mar 18, 2024

No performance tests on ARM ran and the logs don't make it really clear what went wrong. In my local testing on ARM (after merging from master and installing clickhouse-driver and scipy from pip), they ran successfully.

I'll merge from master once more.

@rschu1ze
Copy link
Member Author

#61519 is missing, merging from master again.

@rschu1ze
Copy link
Member Author

Risc-v build fails, a fix is on the way.

This and this performance tests complain

Run Errors
Test	Error
dotProduct	Traceback (most recent call last):
dotProduct	File "/workspace/right/scripts/perf.py", line 430, in

As far as I see in the logs, this is because a new query was added to tests/performance/dotProduct.xml, which runs too slow in the old version

Traceback (most recent call last):
│ File "/workspace/right/scripts/perf.py", line 430, in <module>
│   res = c.execute(
│ File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/client.py", line 267, in execute
│   rv = self.process_ordinary_query(
│ File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/client.py", line 453, in process_ordinary_query
│   return self.receive_result(with_column_types=with_column_types,
│ File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/client.py", line 117, in receive_result
│   return result.get_result()
│ File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/result.py", line 50, in get_result
│   for packet in self.packet_generator:
│ File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/client.py", line 133, in packet_generator
│   packet = self.receive_packet()
│ File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/client.py", line 150, in receive_packet
│   raise packet.exception
clickhouse_driver.errors.ServerException: Code: 159.
dotProduct.query3.run0: DB::Exception: Timeout exceeded: elapsed 15.296030259 seconds, maximum: 15. Stack trace:

but because of this PR, runtime improved to 0.23 sec which is below the maximum query runtime. Also, local performance test execution on x86 and ARM were good.

Let me merge from a master once more and merge as soon as the button is no longer greyed out.

@rschu1ze
Copy link
Member Author

I was too slow and missed the short timeframe in which the Merge button was not greyed out. Another try, sorry.

@rschu1ze rschu1ze merged commit 80e195a into ClickHouse:master Mar 20, 2024
8 of 10 checks passed
robot-ch-test-poll added a commit that referenced this pull request Mar 20, 2024
…ed9f6a0d88e3d7ea8eebd238ba9ca

Cherry pick #60928 to 24.2: Less copying and a correctness fix for `dotProduct`
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 20, 2024
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Mar 20, 2024
rschu1ze added a commit that referenced this pull request Mar 20, 2024
Backport #60928 to 24.2: Less copying and a correctness fix for `dotProduct`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-cloud pr-performance Pull request with some performance improvements pr-synced-to-cloud The PR is synced to the cloud repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants