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

Optimization of __serial_merge function #1970

Merged
merged 18 commits into from
Dec 18, 2024
Merged

Conversation

SergeyKopienko
Copy link
Contributor

In this PR we make optimization of __serial_merge function.
The justification: previous implementation is too complicated:

  • a lot of conditions;
  • a lot of for-loops (and one inside another).

With this new implementation of __serial_merge function we have significant performance profit for merge and merge_sort algorithms on all data sizes.

…optimization of __serial_merge function

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/serial_merge branch from bcc6675 to d3d863d Compare December 17, 2024 17:21
Copy link
Contributor

@MikeDvorskiy MikeDvorskiy left a comment

Choose a reason for hiding this comment

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

Looks very good to me, excepting one minor comment, see above.

…fix review comment: The uninitialized variables...

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
MikeDvorskiy
MikeDvorskiy previously approved these changes Dec 18, 2024
Copy link
Contributor

@MikeDvorskiy MikeDvorskiy left a comment

Choose a reason for hiding this comment

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

Looks very good to me.

Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

It also looks good to me. Below are some minor suggestions.

…fix review comment: __rng3_idx is redefined in the loop below.

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
…fix review comment: Let's use _Index instead of auto, because the type is known.

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
…fix review comment: A nitpick: __rng1_idx_less__n1 -> __rng1_idx_less_n1

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
…fix review comment: rewrite to one ternary operator

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
…fix review comment: rewrite to one ternary operator with additional ( and )

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
…fix self review comment: declare __start1 and __start2 as const

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
mmichel11
mmichel11 previously approved these changes Dec 18, 2024
Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

LGTM on my side.

….h - fix review comment: remove auto for __find_start_point return type

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
danhoeflinger
danhoeflinger previously approved these changes Dec 18, 2024
Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM pending green CI, thanks

mmichel11
mmichel11 previously approved these changes Dec 18, 2024
….h - apply GitHUB clang format

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
@SergeyKopienko
Copy link
Contributor Author

Unfortunately clang-formatting has been fixed. So approves required again.
Sorry for inconveniences.

@SergeyKopienko SergeyKopienko merged commit e65fcd2 into main Dec 18, 2024
22 checks passed
@SergeyKopienko SergeyKopienko deleted the dev/skopienko/serial_merge branch December 18, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants