Skip to content

bugfix: decode prefermance issue fix #375

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nuclearg
Copy link

What this PR does:
decode.go 里面的 Decode() 每次在解析map或list时都会触发一次对 d.refHolders 的轮询 notify(),在解析复杂对象时(我的场景需要处理约10M的数据),这个 Decode() 会被疯狂递归调用,无数次对这个 d.refHolders() 进行无用的 notify(),产生严重的性能问题

我的机器是 MacBook Pro M3Pro 36G,在我本机上,处理10M的数据时,rt将达到5秒。此性能不可接受。

本次改动对 Decode() 的递归次数进行计数,只在最外层的 Decode() 时进行 notify() 操作。

可以通过MR中的 decode_largedata_test.go 验证。此测试用例随机生成一个巨大的map/list相互嵌套的结构并执行序列化/反序列化操作。未引入我的修改时,反序列化的rt远高于序列化(秒级)。引入此修改后序列化与反序列化rt持平(毫秒级)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@nuclearg nuclearg changed the title bugfix: decode prefermance fix bugfix: decode prefermance issue fix Mar 31, 2025
@AlexStocks AlexStocks requested review from wongoo and Copilot and removed request for wongoo April 1, 2025 01:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a performance bug in the Decode() function where repeated unnecessary calls to notify() on d.refHolders are causing significant slowdowns when processing large, complex data structures. The changes introduce a recursive depth counter in Decode() so that d.refHolders.notify() is only called at the outermost level.

  • Introduced a decodeRecursiveDepth counter in the Decoder struct
  • Updated Decode() to increment/decrement this counter and conditionally call notify()
  • Added a large data test case (decode_largedata_test.go) to validate the performance improvement

Reviewed Changes

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

File Description
decode_largedata_test.go Added a test to generate and verify performance with large data
decode.go Updated Decode() with a recursive depth counter to control notify() calls
Comments suppressed due to low confidence (2)

decode.go:243

  • [nitpick] The variable name 'decodeRecursiveDepth' is descriptive, but consider a shorter variant like 'recursiveDepth' if it would improve consistency and readability within the codebase.
d.decodeRecursiveDepth++

decode.go:242

  • If the Decoder type might be used concurrently, please ensure that proper thread-safety measures are in place for 'decodeRecursiveDepth' to avoid potential race conditions.
func (d *Decoder) Decode() (interface{}, error) {

@tiltwind
Copy link
Contributor

tiltwind commented Apr 9, 2025

@nuclearg 感谢

  1. 有一个错误要解决
Error: decode_benchmark_test.go:54:2: ineffectual assignment to `now` (ineffassign)
	now = time.Now()
  1. 测试和压测可以考虑改一下:
func TestMultipleLevelRecursiveDep(t *testing.T) {

	// TODO : 测试 encode 和 decode 后的对象和源对象结构是一致的,值是相等的

}

func BenchmarkMultipleLevelRecursiveDep(b *testing.B) {
	// TODO: 构造一个多层循环依赖的对象, 无需大对象, 压测情况大批量请求可以看出是否有性能改进
	for i := 0; i < b.N; i++ {
		// TODO : 循环 encode 和 decode
	}
}

@nuclearg
Copy link
Author

@tiltwind 基准测试用例已提交

Copy link
Contributor

@wongoo wongoo left a comment

Choose a reason for hiding this comment

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

LGTM

@nuclearg nuclearg requested a review from AlexStocks April 25, 2025 07:14
@AlexStocks
Copy link
Contributor

@nuclearg please fix the ci failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants