Skip to content

[Improvement](memcpy) use assume_aligned to hint aligned memcpy - #60695

Merged
BiteTheDDDDt merged 4 commits into
apache:masterfrom
BiteTheDDDDt:dev_0212_2
Feb 26, 2026
Merged

[Improvement](memcpy) use assume_aligned to hint aligned memcpy#60695
BiteTheDDDDt merged 4 commits into
apache:masterfrom
BiteTheDDDDt:dev_0212_2

Conversation

@BiteTheDDDDt

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

This pull request improves the performance and correctness of fixed-size memory copy operations in hash map key handling by adding alignment-aware logic. The main changes introduce runtime alignment checks and use compiler hints to optimize memory copying, which can help leverage SIMD instructions and avoid undefined behavior due to misaligned accesses.

Hash map key handling improvements:

  • Added runtime checks for memory alignment in MethodKeysFixed to determine whether to use aligned or unaligned memory copy operations, improving safety and performance. [1][2][3]
  • Refactored lambda functions in key handling logic to support alignment-aware memory copying, replacing previous hardcoded alignment assumptions. [1][2][3]

Memory copy utility enhancements:

  • Updated memcpy_fixed in memcpy_small.h to use std::assume_aligned for aligned copies, providing better optimization opportunities for the compiler and ensuring correctness.
  • Added #include <memory> to support std::assume_aligned.

General code safety:

  • Added an early return in insert_keys_into_columns if num_rows == 0 to prevent unnecessary processing.

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

CopilotAI review requested due to automatic review settings February 12, 2026 02:03
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@BiteTheDDDDt

Copy link
Copy Markdown
ContributorAuthor

run buildall

CopilotAI 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.

Pull request overview

This pull request aims to optimize fixed-size memory copy operations in hash map key handling by adding alignment-aware logic using std::assume_aligned. The PR introduces runtime checks to determine whether to use aligned or unaligned memory copy operations, and updates the memcpy_fixed utility function to leverage compiler hints for better SIMD optimization.

Changes:

  • Modified memcpy_fixed to use std::assume_aligned for aligned copies, providing optimization hints to the compiler
  • Refactored hash map key packing and unpacking logic to include runtime alignment checks before calling aligned memcpy
  • Added early return optimization when num_rows is 0

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

FileDescription
be/src/vec/common/memcpy_small.hUpdated memcpy_fixed to use std::assume_aligned for aligned copies; added #include
be/src/vec/common/hash_table/hash_map_context.hAdded runtime alignment checks in pack_fixeds and insert_keys_into_columns; added early return for num_rows == 0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadbe/src/vec/common/hash_table/hash_map_context.h Outdated
}
};
auto foo = [&]<typename Fixed>(Fixed zero) {
if (reinterpret_cast<uintptr_t>(input_keys.data() + pos) % sizeof(Fixed) == 0) {

CopilotAIFeb 12, 2026

Copy link

Choose a reason for hiding this comment

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

The alignment check incorrectly performs pointer arithmetic. The expression input_keys.data() + pos adds pos to a Key* pointer, which moves pos * sizeof(Key) bytes, not pos bytes. However, on line 734, the code correctly uses (char*)(&input_keys[j]) + pos to add pos bytes. The alignment check should also cast to char* before adding pos. The check should be: reinterpret_cast<uintptr_t>((char*)(input_keys.data()) + pos) % sizeof(Fixed) == 0

Suggested change
if (reinterpret_cast<uintptr_t>(input_keys.data() +pos) % sizeof(Fixed) ==0) {
if (reinterpret_cast<uintptr_t>(reinterpret_cast<char*>(input_keys.data()) +pos) % sizeof(Fixed) ==0) {

Copilot uses AI. Check for mistakes.
Comment on lines +614 to +619
auto foo = [&]<typename Fixed>(Fixed zero) {
if (reinterpret_cast<uintptr_t>(result_data + offset) % sizeof(T) == 0) {
goo.template operator()<Fixed, true>(zero);
} else {
goo.template operator()<Fixed, false>(zero);
}

CopilotAIFeb 12, 2026

Copy link

Choose a reason for hiding this comment

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

When aligned=true, memcpy_fixed uses std::assume_aligned on both source and destination pointers, requiring both to be aligned to alignof(Fixed). However, the alignment check only verifies the destination pointer (result_data + offset), not the source pointer (data). If data is not aligned to alignof(Fixed), this creates undefined behavior. The code should either check both pointers for alignment or only use unaligned memcpy when either pointer is misaligned.

Copilot uses AI. Check for mistakes.
Comment on lines +737 to 742
auto foo = [&]<typename Fixed>(Fixed zero) {
if (reinterpret_cast<uintptr_t>(input_keys.data() + pos) % sizeof(Fixed) == 0) {
goo.template operator()<Fixed, true>(zero);
} else {
goo.template operator()<Fixed, false>(zero);
}

CopilotAIFeb 12, 2026

Copy link

Choose a reason for hiding this comment

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

When aligned=true, memcpy_fixed uses std::assume_aligned on both source and destination pointers. However, the alignment check only verifies the source pointer ((char*)(input_keys.data()) + pos), not the destination pointer (data + j * sizeof(Fixed)). If the destination is not aligned to alignof(Fixed), this creates undefined behavior. The code should check both pointers for alignment or use unaligned memcpy when either pointer is misaligned.

Copilot uses AI. Check for mistakes.
@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100.00% (25/25) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage71.77% (25968/36180)
Line Coverage54.42% (271788/499445)
Region Coverage51.76% (225521/435723)
Branch Coverage53.32% (97043/182015)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@BiteTheDDDDt

Copy link
Copy Markdown
ContributorAuthor

run buildallrun buildall

@BiteTheDDDDt

Copy link
Copy Markdown
ContributorAuthor

run buildall

HappenLee
HappenLee previously approved these changes Feb 21, 2026

@HappenLeeHappenLee 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.

LGTM

@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actionsgithub-actionsBot added approved Indicates a PR has been approved by one committer. reviewed labels Feb 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 48.00% (12/25) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage52.56% (19545/37183)
Line Coverage36.14% (182214/504126)
Region Coverage32.52% (141459/435040)
Branch Coverage33.47% (61289/183108)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 48.00% (12/25) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage57.26% (20866/36442)
Line Coverage40.29% (202622/502887)
Region Coverage37.16% (163289/439425)
Branch Coverage37.73% (69361/183812)

@BiteTheDDDDt

Copy link
Copy Markdown
ContributorAuthor

run buildall

mrhhsg
mrhhsg previously approved these changes Feb 24, 2026

@mrhhsgmrhhsg 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

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 48.00% (12/25) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage57.16% (20854/36486)
Line Coverage40.20% (202222/503087)
Region Coverage37.05% (162959/439787)
Branch Coverage37.58% (69137/183976)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 48.00% (12/25) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage57.18% (20865/36490)
Line Coverage40.20% (202402/503550)
Region Coverage37.04% (163059/440197)
Branch Coverage37.57% (69195/184179)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 48.00% (12/25) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage57.14% (20850/36490)
Line Coverage40.19% (202358/503550)
Region Coverage36.81% (162021/440197)
Branch Coverage37.51% (69084/184179)

1 similar comment
@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 48.00% (12/25) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage57.14% (20850/36490)
Line Coverage40.19% (202358/503550)
Region Coverage36.81% (162021/440197)
Branch Coverage37.51% (69084/184179)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100.00% (25/25) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage63.34% (23111/36490)
Line Coverage46.59% (234625/503550)
Region Coverage43.70% (192383/440197)
Branch Coverage44.85% (82606/184179)

1 similar comment
@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100.00% (25/25) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage63.34% (23111/36490)
Line Coverage46.59% (234625/503550)
Region Coverage43.70% (192383/440197)
Branch Coverage44.85% (82606/184179)

@BiteTheDDDDt
BiteTheDDDDt dismissed stale reviews from mrhhsg and HappenLee via 3b853dfFebruary 25, 2026 04:05
@BiteTheDDDDt

Copy link
Copy Markdown
ContributorAuthor

run buildall

@github-actionsgithub-actionsBot removed the approved Indicates a PR has been approved by one committer. label Feb 25, 2026
@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 46.43% (13/28) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage52.53% (19575/37265)
Line Coverage36.14% (182568/505194)
Region Coverage32.51% (141805/436166)
Branch Coverage33.44% (61419/183660)

@BiteTheDDDDt

Copy link
Copy Markdown
ContributorAuthor

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100.00% (32/32) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage71.60% (26134/36498)
Line Coverage54.33% (273652/503694)
Region Coverage51.76% (227923/440334)
Branch Coverage53.11% (97852/184246)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 45.16% (14/31) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage52.56% (19603/37293)
Line Coverage36.18% (182894/505527)
Region Coverage32.50% (141881/436550)
Branch Coverage33.45% (61485/183818)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100.00% (31/31) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage73.27% (26763/36526)
Line Coverage56.54% (284964/504027)
Region Coverage53.98% (237884/440718)
Branch Coverage55.63% (102580/184404)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100.00% (31/31) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage73.27% (26763/36526)
Line Coverage56.53% (284925/504027)
Region Coverage53.96% (237813/440718)
Branch Coverage55.61% (102551/184404)

@HappenLeeHappenLee 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.

LGTM

@mrhhsgmrhhsg 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

@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actionsgithub-actionsBot added the approved Indicates a PR has been approved by one committer. label Feb 26, 2026
@BiteTheDDDDt
BiteTheDDDDt merged commit d7ae570 into apache:masterFeb 26, 2026
29 of 31 checks passed
yiguolei pushed a commit to yiguolei/incubator-doris that referenced this pull request Mar 23, 2026
…he#60695)
This pull request improves the performance and correctness of fixed-size
memory copy operations in hash map key handling by adding
alignment-aware logic. The main changes introduce runtime alignment
checks and use compiler hints to optimize memory copying, which can help
leverage SIMD instructions and avoid undefined behavior due to
misaligned accesses.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approvedIndicates a PR has been approved by one committer.dev/4.1.0-mergedreviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@BiteTheDDDDt@hello-stephen@mrhhsg@HappenLee@yiguolei