Skip to content

columnToFieldIndex: Improve performance #406

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

Closed
wants to merge 5 commits into from

Conversation

paulquerna-okta
Copy link

This PR is in the same spirit as #347

We found that the function columnToFieldIndex was inefficient from a CPU usage POV, but more importantly created ton of objects for the GC.

Analyzing real world usage of gorp in a proprietary API server, we were able to reduce overall memory allocs by ~50% and bytes allocated by ~20% with this patch.

This PR:

  • Adds a few tests and a micro-benchmark for columnToFieldIndex
  • Takes a dependency on reflectx to implement columnToFieldIndex. reduce call to reflect.Type.FieldByName #347 tries to implement it locally, but reflectx does exactly what we need here, and has built-in caching based on the reflect.Type. We would need to replicate a few things into gorp if the dependency is not acceptable.

Micro-benchmark results:

benchmark                          old ns/op     new ns/op     delta
BenchmarkCcolumnToFieldIndex-8     7183          334           -95.35%

benchmark                          old allocs     new allocs     delta
BenchmarkCcolumnToFieldIndex-8     77             2              -97.40%

benchmark                          old bytes     new bytes     delta
BenchmarkCcolumnToFieldIndex-8     1256          208           -83.44%

@paulquerna-okta
Copy link
Author

(note, I think we need some improved tests and changes for embedded structures, but I hope this is still a directionally interesting PR)

@hi-wayne
Copy link

hi-wayne commented Jan 25, 2021

sqlx 1.2 reflectx not suppor embedded structs
fyi sqlx1.3 release
jmoiron/sqlx#429

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