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

enhancement: Avoid calling reflect.New() when passing in slice of values to Scan() #5388

Merged

Conversation

Bexanderthebex
Copy link
Contributor

What did this pull request do?

Tries to optimize Scan() method by recycling a struct when a user passes a slice of values to prevent calling reflect.New() every time regardless of the type.

User Case Description

When passing a slice of values, the run time is the same as passing in a slice of pointers. Users should not be penalized for such.

@Bexanderthebex
Copy link
Contributor Author

Relevant: #5372

user := *GetUser("scan", Config{})
DB.Create(&user)

var u User

Choose a reason for hiding this comment

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

Suggested change
var u User
b.ResetTimer()
var u User

DB.Create(&user)
}

var u []User

Choose a reason for hiding this comment

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

Suggested change
var u []User
b.ResetTimer()
var u []User

DB.Raw("select * from users").Scan(&u)
}
}

Choose a reason for hiding this comment

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

Suggested change
func BenchmarkScanSlicePointer(b *testing.B) {
for i := 0; i < 10_000; i++ {
user := *GetUser(fmt.Sprintf("scan-%d", i), Config{})
DB.Create(&user)
}
b.ResetTimer()
var u []*User
for x := 0; x < b.N; x++ {
DB.Raw("select * from users").Scan(&u)
}
}

@umitanuki
Copy link

umitanuki commented May 30, 2022

My benchmark result suggests something promising.

BenchmarkScanSlice-12           	   9	 188888672 ns/op	66993973 B/op	 1011373 allocs/op
BenchmarkScanSlicePointer-12    	   3	 355558214 ns/op	61715941 B/op	 2051542 allocs/op

@Bexanderthebex
Copy link
Contributor Author

Bexanderthebex commented May 31, 2022

@umitanuki Agreed, benchmark according to my benchmark, improvement is ~50%

BenchmarkScanSlice-12                  8         213268227 ns/op        68680647 B/op    1092749 allocs/op
BenchmarkScanSlicePointer-12      4         370261906 ns/op        79490372 B/op    2644559 allocs/op

tests/benchmark_test.go Outdated Show resolved Hide resolved
@Bexanderthebex
Copy link
Contributor Author

New:

BenchmarkScanSlice-12             14          73788941 ns/op        31725140 B/op     499899 allocs/op
BenchmarkScanSlicePointer-12 15          69792858 ns/op        15323992 B/op     509812 allocs/op

Old:

BenchmarkScanSlice-12               13          83697560 ns/op        36216732 B/op     509938 allocs/op
BenchmarkScanSlicePointer-12   14          74349205 ns/op        15321940 B/op     509806 allocs/op

cc: @a631807682

@Bexanderthebex
Copy link
Contributor Author

@jinzhu when you have time, can you take a look? thank you!

scan.go Outdated
@@ -261,7 +262,11 @@ func Scan(rows Rows, db *DB, mode ScanMode) {
}
}
} else {
elem = reflect.New(reflectValueType)
if isPtr {
Copy link
Member

Choose a reason for hiding this comment

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

We can change this to if isPtr && db.RowsAffected > 0?

Seems not necessary to create a new struct for the first record.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jinzhu do you mean if isPtr && db.RowsAffected == 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no, you are correct, making this change now.

@jinzhu
Copy link
Member

jinzhu commented Jun 1, 2022

@jinzhu when you have time, can you take a look? thank you!

Just submitted a small review.

In general looks great, thank you for your contribution. ❤️

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