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

perf(depinject): optimize isExportedType function #23857

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0x2d3c
Copy link
Contributor

@0x2d3c 0x2d3c commented Feb 28, 2025

  • avoid redundant rune slice creation in unicode check

@0x2d3c 0x2d3c requested a review from a team as a code owner February 28, 2025 16:22
@aljo242
Copy link
Collaborator

aljo242 commented Mar 3, 2025

@0x2d3c what is the performance impact of this?

Why is this faster and can you show benchmarks verifying that there is improvement?

@0x2d3c
Copy link
Contributor Author

0x2d3c commented Mar 3, 2025

what is the performance impact of this?

[]rune(name) will initialize a slice of the same length as name, resulting in additional memory allocation. The implementation of utf8.DecodeRuneInString just meets the actual scenario here, avoiding additional memory overhead

@technicallyty technicallyty changed the title feat(depinject): optimize isExportedType function for performance perf(depinject): optimize isExportedType function Mar 3, 2025
@0x2d3c
Copy link
Contributor Author

0x2d3c commented Mar 3, 2025

Why is this faster and can you show benchmarks verifying that there is improvement?

short string bench output:

BenchmarkUsingRuneSlice-8 25518284 46.3 ns/op 48 B/op 1 allocs/op
BenchmarkUsingDecodeRune-8 86976563 13.6 ns/op 0 B/op 0 allocs/op

long string bench output:

BenchmarkUsingRuneSlice-8 12938479 92.8 ns/op 96 B/op 1 allocs/op
BenchmarkUsingDecodeRune-8 86932154 13.7 ns/op 0 B/op 0 allocs/op

@technicallyty
Copy link
Contributor

what is the performance impact of this?

[]rune(name) will initialize a slice of the same length as name, resulting in additional memory allocation. The implementation of utf8.DecodeRuneInString just meets the actual scenario here, avoiding additional memory overhead

wouldn't it be even faster to just do:

if r := name[0]; unicode.IsLower(rune(r))?

@0x2d3c
Copy link
Contributor Author

0x2d3c commented Mar 3, 2025

what is the performance impact of this?

[]rune(name) will initialize a slice of the same length as name, resulting in additional memory allocation. The implementation of utf8.DecodeRuneInString just meets the actual scenario here, avoiding additional memory overhead

wouldn't it be even faster to just do:

if r := name[0]; unicode.IsLower(rune(r))?

does not support non-ASCII characters (multi-byte UTF-8 characters will be truncated)

@technicallyty
Copy link
Contributor

Why is this faster and can you show benchmarks verifying that there is improvement?

short string bench output:

BenchmarkUsingRuneSlice-8 25518284 46.3 ns/op 48 B/op 1 allocs/op BenchmarkUsingDecodeRune-8 86976563 13.6 ns/op 0 B/op 0 allocs/op

long string bench output:

BenchmarkUsingRuneSlice-8 12938479 92.8 ns/op 96 B/op 1 allocs/op BenchmarkUsingDecodeRune-8 86932154 13.7 ns/op 0 B/op 0 allocs/op

can you provide your benchmarking code?

@0x2d3c
Copy link
Contributor Author

0x2d3c commented Mar 6, 2025

Why is this faster and can you show benchmarks verifying that there is improvement?

short string bench output:
BenchmarkUsingRuneSlice-8 25518284 46.3 ns/op 48 B/op 1 allocs/op BenchmarkUsingDecodeRune-8 86976563 13.6 ns/op 0 B/op 0 allocs/op
long string bench output:
BenchmarkUsingRuneSlice-8 12938479 92.8 ns/op 96 B/op 1 allocs/op BenchmarkUsingDecodeRune-8 86932154 13.7 ns/op 0 B/op 0 allocs/op

can you provide your benchmarking code?

this is the bench code for two ways to implement local verification and comparison. I don't know if it has any reference value.

// short(ASCII)
var shortASCII = "hi"

// short(multi-byte UTF-8)
var shortUTF8 = "你好"

// long(ASCII)
var longASCII = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"

// long (multi-byte UTF-8)
var longUTF8 = "你好,世界!🚀🔥"

func BenchmarkRuneShortASCII(b *testing.B) {
	for i := 0; i < b.N; i++ {
		r, _ := utf8.DecodeRuneInString(shortASCII)
		_ = unicode.IsLower(r)
	}
}

func BenchmarkRuneSliceShortASCII(b *testing.B) {
	for i := 0; i < b.N; i++ {
		unicode.IsLower([]rune(shortASCII)[0])
	}
}

func BenchmarkRuneShortUTF8(b *testing.B) {
	for i := 0; i < b.N; i++ {
		r, _ := utf8.DecodeRuneInString(shortUTF8)
		_ = unicode.IsLower(r)
	}
}

func BenchmarkRuneSliceShortUTF8(b *testing.B) {
	for i := 0; i < b.N; i++ {
		unicode.IsLower([]rune(shortUTF8)[0])
	}
}

func BenchmarkRuneLongASCII(b *testing.B) {
	for i := 0; i < b.N; i++ {
		r, _ := utf8.DecodeRuneInString(longASCII)
		_ = unicode.IsLower(r)
	}
}

func BenchmarkRuneSliceLongASCII(b *testing.B) {
	for i := 0; i < b.N; i++ {
		unicode.IsLower([]rune(longASCII)[0])
	}
}

func BenchmarkRuneLongUTF8(b *testing.B) {
	for i := 0; i < b.N; i++ {
		r, _ := utf8.DecodeRuneInString(longUTF8)
		_ = unicode.IsLower(r)
	}
}

func BenchmarkRuneSliceLongUTF8(b *testing.B) {
	for i := 0; i < b.N; i++ {
		unicode.IsLower([]rune(longUTF8)[0])
	}
}

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.

3 participants