Skip to content

[optimization] avoid extra memory copy while build hash table - #5301

Merged
morningman merged 3 commits into
apache:masterfrom
stdpain:optimize_hash_table_build
Jan 30, 2021
Merged

[optimization] avoid extra memory copy while build hash table#5301
morningman merged 3 commits into
apache:masterfrom
stdpain:optimize_hash_table_build

Conversation

@stdpain

@stdpainstdpain commented Jan 26, 2021

Copy link
Copy Markdown
Contributor

Proposed changes

avoid extra memory copy while build hash table

reference to #5300

Types of changes

  • [] Bugfix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • [] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [] Documentation Update (if none of the other choices apply)
  • Code refactor (Modify the code structure, format the code, etc...)

Checklist

  • I have created an issue on (Fix #ISSUE) and described the bug/feature there in detail
  • Compiling and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged

Further comments

This modification may have a performance improvement of more than 25% during the hash table construction phase.

@stdpain
stdpainforce-pushed the optimize_hash_table_build branch from 7b86ef7 to 4c102eaCompareJanuary 26, 2021 06:00
wangbo
wangbo reviewed Jan 26, 2021
View reviewed changes
Comment threadbe/src/exec/hash_table.hpp
wangbo
wangbo reviewed Jan 26, 2021
View reviewed changes
Comment threadbe/src/exec/hash_table.cpp
wangbo
wangbo reviewed Jan 26, 2021
View reviewed changes
Comment threadbe/src/exec/hash_table.h
wangbo
wangbo reviewed Jan 27, 2021
View reviewed changes
Comment threadbe/src/exec/hash_table.cpp Outdated
wangbo
wangbo reviewed Jan 27, 2021
View reviewed changes
Comment threadbe/src/exec/hash_table.hpp Outdated
wangbo
wangbo reviewed Jan 27, 2021
View reviewed changes
Comment threadbe/src/exec/hash_table.h
// Buffer to store node data.
uint8_t* _current_nodes;
// number of nodes stored (i.e. size of hash table)
int64_t _num_nodes;

@wangbowangboJan 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
int64_t_num_nodes;
int64_t_total_used;

Easier to understand

stdpainand others added 2 commits January 28, 2021 10:54
@stdpain@wangbo
Update be/src/exec/hash_table.cpp
72730df
Co-authored-by: wangbo <506340561@qq.com>
@stdpain
Update hash_table.hpp
b85a109
yangzhg
yangzhg approved these changes Jan 28, 2021
View reviewed changes

@yangzhgyangzhg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wangbowangbo added the area/sql/execution Issues or PRs related to the execution engine label Jan 28, 2021
wangbo
wangbo approved these changes Jan 28, 2021
View reviewed changes

@wangbowangbo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@morningman
morningman merged commit bf0cb78 into apache:masterJan 30, 2021
@yangzhgyangzhg mentioned this pull request Feb 9, 2021
Closed
@stdpain
stdpain deleted the optimize_hash_table_build branch February 21, 2021 17:01
EmmyMiao87 pushed a commit to EmmyMiao87/incubator-doris that referenced this pull request Apr 21, 2021
@stdpain@morningman-cmy
[optimization] avoid extra memory copy while build hash table (apache…
046d583
…#5301)
avoid extra memory copy while build hash table
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yangzhgyangzhgyangzhg approved these changes
@wangbowangbowangbo approved these changes

Assignees

No one assigned

Labels

area/sql/executionIssues or PRs related to the execution engine

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@stdpain@yangzhg@wangbo@morningman