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

Fix sparse_hashed dict performance with sequential keys (wrong hash function) #32536

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

azat
Copy link
Collaborator

@azat azat commented Dec 10, 2021

Changelog category (leave one):

  • Bug Fix (user-visible misbehaviour in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix sparse_hashed dict performance with sequential keys (wrong hash function)

Detailed description / Documentation draft:
In #27152 the hash function for sparse_hash_map had been changed to
std::hash<> switch it back to DefaultHash<> (ClickHouse builtin), since
std::hash<> for numeric keys returns itself and this does not works
great with sparse_hash_map.

I've tried the example from #32480 and using some hash fixes the
performance of sparse_hashed layout.

Fixes: #32480
Fixes: #27152
Backport: 20.9+

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Dec 10, 2021
@alexey-milovidov alexey-milovidov added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Dec 10, 2021
@alexey-milovidov alexey-milovidov self-assigned this Dec 10, 2021
@alexey-milovidov
Copy link
Member

Special thanks for adding performance test! 👍

@azat azat marked this pull request as draft December 11, 2021 08:03
@alexey-milovidov
Copy link
Member

@alexey-milovidov
Copy link
Member

PS. For our own HashTables, we should use crc32c.

@alexey-milovidov
Copy link
Member

We should completely revert #27152 and remove useless files, because we don't use "Arcadia" build system anymore.

@azat
Copy link
Collaborator Author

azat commented Dec 11, 2021

@azat Performance test showed significant performance degradation: https://clickhouse-test-reports.s3.yandex.net/32536/6d31a389f12837f3b8168f17217dadab4f4f4974/performance_comparison/report.html#fail1

Yes, the problem is that for sequential keys DefaultHash does not works great, but it works significantly better if those sequential keys are randomly accessed/inserted, I'm going to update performance test for now (to fix the original issue) and get back to this later.

We should completely revert #27152 and remove useless files, because we don't use "Arcadia" build system anymore.

@azat azat force-pushed the sparse_hashed-dict-fix branch 2 times, most recently from 9a4494a to f4fdea7 Compare December 12, 2021 12:33
@azat
Copy link
Collaborator Author

azat commented Dec 13, 2021

So now perf tests shows improvement, but the query itself is too slow (there is lots of heuristics in report.py, that does not allow the query to run >~2 seconds each):

Old, s New, s Ratio of speedup (-) or slowdown (+) Relative difference (new − old) / old p < 0.01 threshold Test # Query
12.281 5.111 -2.402x -0.584 0.578 hashed_dictionary 1 SYSTEM RELOAD DICTIONARY SPARSE_HASHED_dictionary

Will rework the perf test.

@azat
Copy link
Collaborator Author

azat commented Dec 13, 2021

Actually is it not that easy to satisfy all the conditions (various timeouts for each query and test overall) for performance tests and write one for this issue, so let's do this separately.

Perf test had been removed for now.

@azat azat marked this pull request as ready for review December 13, 2021 20:30
@alexey-milovidov
Copy link
Member

Could you please remove the SparseHashMap.h file?
It was added for "arcadia" and we don't use arcadia anymore.

Also need to figure out what is wrong with integration tests...

@azat
Copy link
Collaborator Author

azat commented Dec 14, 2021

https://s3.amazonaws.com/clickhouse-test-reports/32536/559b01a2e43d09571195fb1cc08d83133f559203/integration_tests__release__actions__[2/2].html

  • test_merge_tree_blob_storage
E               compose.cli.verbose_proxy.proxy_callable: docker pull <- ('mcr.microsoft.com/azure-storage/azurite', tag='latest', stream=True, platform=None)
E               compose.cli.errors.log_api_error: Get "https://mcr.microsoft.com/v2/": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)

https://s3.amazonaws.com/clickhouse-test-reports/32536/559b01a2e43d09571195fb1cc08d83133f559203/integration_tests__asan__actions__[2/3].html

https://s3.amazonaws.com/clickhouse-test-reports/32536/559b01a2e43d09571195fb1cc08d83133f559203/integration_tests__asan__actions__[1/3].html

https://s3.amazonaws.com/clickhouse-test-reports/32536/559b01a2e43d09571195fb1cc08d83133f559203/integration_tests__release__actions__[1/2].html

…unction)

In ClickHouse#27152 the hash function for sparse_hash_map had been changed to
std::hash<> switch it back to DefaultHash<> (ClickHouse builtin), since
std::hash<> for numeric keys returns itself and this does not works
great with sparse_hash_map.

I've tried the example from ClickHouse#32480 and using some hash fixes the
performance of sparse_hashed layout.

Fixes: ClickHouse#32480

v2: Add comments for SparseHashMap
It was added only for arcadia build, and used only in one place, no need
to have a separate typedef for it.
@azat
Copy link
Collaborator Author

azat commented Dec 14, 2021

https://github.com/ClickHouse/ClickHouse/runs/4516380764?check_suite_focus=true

  Error: fatal: Unable to create '/home/ubuntu/actions-runner/_work/ClickHouse/ClickHouse/.git/modules/contrib/aws/index.lock': File exists.
  
  Another git process seems to be running in this repository, e.g.
  an editor opened by 'git commit'. Please make sure all processes
  are terminated then try again. If it still fails, a git process
  may have crashed in this repository earlier:
  remove the file manually to continue.
...
  Unable to checkout '00b03604543367d7e310cb0993973fdcb723ea79' in submodule path 'contrib/aws'
  Error: The process '/usr/bin/git' failed with exit code 1

Something left from the previous run in this workspace?

@azat azat deleted the sparse_hashed-dict-fix branch December 14, 2021 18:46
robot-clickhouse pushed a commit that referenced this pull request Dec 14, 2021
robot-clickhouse pushed a commit that referenced this pull request Dec 14, 2021
alexey-milovidov added a commit that referenced this pull request Dec 15, 2021
Backport #32536 to 21.12: Fix sparse_hashed dict performance with sequential keys (wrong hash function)
kitaisreal added a commit that referenced this pull request Dec 15, 2021
Backport #32536 to 21.11: Fix sparse_hashed dict performance with sequential keys (wrong hash function)
@Felixoid Felixoid added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jul 14, 2022
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-bugfix Pull request with bugfix, not backported by default pr-must-backport Pull request should be backported intentionally. Use this label with great care!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hierarchical dictionaries with sparse_hashed layout can't be loaded
5 participants