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

planner: care to use errors.new and errors.errorf as your warnings and notes #49291

Closed
AilinKid opened this issue Dec 8, 2023 · 4 comments · Fixed by #49390, #49581, #49600, #49686 or #49714
Closed
Assignees
Labels
affects-7.1 This bug affects the 7.1.x(LTS) versions. report/customer Customers have encountered this bug. severity/moderate sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement.

Comments

@AilinKid
Copy link
Contributor

AilinKid commented Dec 8, 2023

Enhancement

github/pingcap/errors.git has their own encapsulation of errors.new and errors.errorf, in which their will fetch stack trace of currently places. If your generation of this errors is used like warning and note, inside which the stack trace is never necessary, so try to avoid usage of this or use another stack-less errors

@AilinKid AilinKid added type/enhancement The issue or PR belongs to an enhancement. sig/planner SIG: Planner labels Dec 8, 2023
@AilinKid AilinKid self-assigned this Dec 8, 2023
@siddontang
Copy link
Member

@AilinKid

can you give some examples about which places should not use this so others can help you improve them?

and can you also show an example of how to use a stack-less erros?

@AilinKid
Copy link
Contributor Author

AilinKid commented Dec 11, 2023

image

红色圈圈部分,我们在 v6.5.0 上是看不到的,为什么会在 v7.1.0 上成为性能问题呢(虽然可能整体来说占比不是很大,导致 planner 部分增长了(18.5-15.3)约占 TiDB 的 3 个点,占 planner 自身的(18.5-15.3)/18.5  = 17 个点)。研究发现,这部分基本上都是系统调用 caller的部分,主要是服务与 error.New() 和 errors.Errorf() 里面的填充的 stack 字段部分(stack: callers())。这部分代码是 7.1 新增用来 log warning 或者 note 用的。

we did find a performance problem from internal incall concluded by a lark document in planner component, you can search the keyword by Care to use errors.New and errors.Errorf in lark app, which is not suitable to put the link here.

stack-less error is called like errors.NewNoStackError("msg")

ti-chi-bot bot pushed a commit that referenced this issue Dec 14, 2023
@AilinKid AilinKid reopened this Dec 19, 2023
AilinKid added a commit to AilinKid/tidb that referenced this issue Dec 22, 2023
@ti-chi-bot ti-chi-bot added the affects-7.1 This bug affects the 7.1.x(LTS) versions. label Dec 22, 2023
ti-chi-bot bot pushed a commit that referenced this issue Dec 22, 2023
@jackysp
Copy link
Member

jackysp commented Apr 21, 2024

image In version 7.1, I saw that in the batch write scenario, the CPU usage can reach up to 25%.

@seiya-annie
Copy link

/found customer

@ti-chi-bot ti-chi-bot bot added the report/customer Customers have encountered this bug. label Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment