mathutil: replace mathutil functions with Go built-in functions #56620

Anudhyan · 2024-10-12T12:44:51Z

What problem does this PR solve?

Issue Number: close #56594

Problem Summary:

Swapping out mathutil functions in favor of Go's built-in ones, like trading in a custom Max function for Go's math.Max 🛠️.

What changed and how does it work?

The code now uses Go's built-in functions (bye-bye mathutil), making things simpler and cleaner.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
- Gave it a good ol' manual test to make sure everything runs smooth like butter with the built-in functions.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

sre-bot · 2024-10-12T12:44:58Z

All committers have signed the CLA.

ti-chi-bot · 2024-10-12T12:45:00Z

Welcome @Anudhyan!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to pingcap/tidb. 😃

ti-chi-bot · 2024-10-12T12:45:00Z

Hi @Anudhyan. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ti-chi-bot · 2024-10-12T12:45:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign overvenus for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tiprow · 2024-10-12T12:45:08Z

Hi @Anudhyan. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ti-chi-bot · 2024-10-12T12:56:26Z

@Anudhyan: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tiprow · 2024-10-12T12:56:33Z

@Anudhyan: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Anudhyan · 2024-10-12T12:58:37Z

@b41sh please check this and merge this , if possible

Anudhyan · 2024-10-12T13:04:36Z

@hawkingrei please check this and merge this, if possible

lance6716 · 2024-10-12T13:28:33Z

pkg/util/mathutil/math.go

 )

-// Abs implement the abs function according to http://cavaliercoder.com/blog/optimized-abs-for-int64-in-go.html


the old code is an optimized version. Please don't change it.

lance6716 · 2024-10-12T13:29:57Z

pkg/util/mathutil/math.go

 		return minv
 	}
 	return n
 }

-// NextPowerOfTwo returns the smallest power of two greater than or equal to `i`
-// Caller should guarantee that i > 0 and the return value is not overflow.
+// NextPowerOfTwo returns the smallest power of two greater than or equal to `i`.
 func NextPowerOfTwo(i int64) int64 {


I'm not sure the performance of these two versions. If you want to change it, please post some benchmark tests

lance6716 · 2024-10-12T13:31:16Z

pkg/util/mathutil/math.go

 }

-// Max returns the largest one from its arguments.
+// Max returns the largest element from its arguments.
 func Max[T constraints.Ordered](x T, xs ...T) T {


The original issue wants to remove this function. Caller can use the built-in max function instead

Anudhyan · 2024-10-13T13:45:30Z

/retest

ti-chi-bot · 2024-10-13T13:45:45Z

@Anudhyan: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tiprow · 2024-10-13T13:45:50Z

@Anudhyan: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ti-chi-bot · 2024-10-25T10:38:30Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Update math.go

9c29f6e

ti-chi-bot bot added do-not-merge/invalid-title do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. labels Oct 12, 2024

ti-chi-bot bot added needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 12, 2024

Anudhyan marked this pull request as draft October 12, 2024 12:45

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked labels Oct 12, 2024

Anudhyan changed the title ~~Update math.go~~ mathutil: replace mathutil functions with Go built-in functions Oct 12, 2024

ti-chi-bot bot removed the do-not-merge/invalid-title label Oct 12, 2024

Anudhyan marked this pull request as ready for review October 12, 2024 12:53

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 12, 2024

lance6716 added the hacktoberfest label Oct 12, 2024

lance6716 reviewed Oct 12, 2024

View reviewed changes

Anudhyan marked this pull request as draft October 12, 2024 16:38

ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 12, 2024

Anudhyan marked this pull request as ready for review October 12, 2024 16:57

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 12, 2024

Update math.go

e75c436

ti-chi-bot bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 13, 2024

Update math.go

0f247f0

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 14, 2024

Update math.go

d1e787e

Anudhyan marked this pull request as draft October 24, 2024 16:06

ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2024

Anudhyan marked this pull request as ready for review October 24, 2024 16:08

ti-chi-bot bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mathutil: replace mathutil functions with Go built-in functions #56620

mathutil: replace mathutil functions with Go built-in functions #56620

Anudhyan commented Oct 12, 2024 •

edited

Loading

sre-bot commented Oct 12, 2024 •

edited

Loading

ti-chi-bot bot commented Oct 12, 2024

ti-chi-bot bot commented Oct 12, 2024

ti-chi-bot bot commented Oct 12, 2024

tiprow bot commented Oct 12, 2024

ti-chi-bot bot commented Oct 12, 2024

tiprow bot commented Oct 12, 2024

Anudhyan commented Oct 12, 2024 •

edited

Loading

Anudhyan commented Oct 12, 2024 •

edited

Loading

lance6716 Oct 12, 2024

lance6716 Oct 12, 2024

lance6716 Oct 12, 2024

Anudhyan commented Oct 13, 2024

ti-chi-bot bot commented Oct 13, 2024

tiprow bot commented Oct 13, 2024

ti-chi-bot bot commented Oct 25, 2024

		)

		// Abs implement the abs function according to http://cavaliercoder.com/blog/optimized-abs-for-int64-in-go.html

mathutil: replace mathutil functions with Go built-in functions #56620

Are you sure you want to change the base?

mathutil: replace mathutil functions with Go built-in functions #56620

Conversation

Anudhyan commented Oct 12, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

sre-bot commented Oct 12, 2024 • edited Loading

ti-chi-bot bot commented Oct 12, 2024

ti-chi-bot bot commented Oct 12, 2024

ti-chi-bot bot commented Oct 12, 2024

tiprow bot commented Oct 12, 2024

ti-chi-bot bot commented Oct 12, 2024

tiprow bot commented Oct 12, 2024

Anudhyan commented Oct 12, 2024 • edited Loading

Anudhyan commented Oct 12, 2024 • edited Loading

lance6716 Oct 12, 2024

Choose a reason for hiding this comment

lance6716 Oct 12, 2024

Choose a reason for hiding this comment

lance6716 Oct 12, 2024

Choose a reason for hiding this comment

Anudhyan commented Oct 13, 2024

ti-chi-bot bot commented Oct 13, 2024

tiprow bot commented Oct 13, 2024

ti-chi-bot bot commented Oct 25, 2024

Anudhyan commented Oct 12, 2024 •

edited

Loading

sre-bot commented Oct 12, 2024 •

edited

Loading

Anudhyan commented Oct 12, 2024 •

edited

Loading

Anudhyan commented Oct 12, 2024 •

edited

Loading