Use custom reflection for decoding structs #49
Conversation
This replaces quite a bit of the gocassa internals to return an object for the query and values rather than a string and a variadic interface. This refactoring is for down the line so we can return also the Column Names in the Statement and potentially more data in the future if needed. To achieve this, we've had to tweak the public interface too significantly hence the bump to 2.0.
This PR adds type information to the fields we are selecting if making a SELECT query. This information can be used by consumers to do reflection and allocate the relevant pointers for scanning using the gocql iter.Scan method. To be frankly honest, it's quite a leaky abstraction however it is super optimal because you don't end up doing multiple reflection steps and can support things like custom CQL Unmarshal methods without a lot of work.
Initially I wanted to seperate the concerns of iteration to the backend so it had more control. However, this would mean each backend would need to do reflection business which is hard to test for and hard to assert for correctness. It also is a very leaky abstraction because gocassa would need to expose the field types externally. This is messy and prone to error. Instead, this change passes in a scanner which takes an iterator. We can hook in a mock iterator (as long as it passes the same interface) or a gocql iterator. That way, gocassa is in control of what happens with the iterator for scanning rows. In order for the Scanner to work, we've added a bit more reflection code which will use the cache to ensure that for a given struct type, we only need to do the hardcore work of figuring out fields once.
Similar to the previous commit, we don't want a leaky abstraction around field types when parsing a statement. Also it turns out a lot of programs pass in a completely different struct types during unmarshaling (sometimes with fields being removed or with different types) than the ones passed in during table initialisation. We could've forbidden this using a type check as an optimisation (and that option is still there) but for now it seems quite heavy handed. Because of this, we can get rid of some cruft we added around logging the struct on table creation and generating field types.
Simply a semantic change but now doing a scan and parsing a raw map isn't how we do things nowadays. Types and Structs for days!
This removes the use of the mapstructure library. Now the decode step is embedded as part of the scanner which handles all sorts of pointer magic and putting the pointers into the right places in the struct. Also cleans up a stray use of generateFieldTypes which we got rid of in a prior commit.
The bulk of the change is tweaking the tests which tested the mapstructure decoder to use the scanner instead. Since the scanner depends on an iterator, we've added a mockIterator which looks at the fields and coerces the values from the results map in the order specified.
Adds some sanity checks given it's a publicly exposed interface and the implementation is passed across this interface boundary so it's worth validating we have the implementation correct for future changes.
In an ideal world, we would rip out all of this code and replace it with a call to UnmarshalCQL with some crafted params. This will give us the benefit of all the checks that gocql has built in. Either way, this PR locks down the current behaviour so when we do the migration in the future, we can assert for major changes.
Part of this work involved some new functions in the reflect side of things to take a struct and turn it into a map. We've also changed the semantics of Field (especially now that it's public) to not automatically follow pointers. If a struct type is *int, that shouldn't be represented as int as the type.
This commit is the cleanup commit. We've all been there, you've been prototyping and in the middle of getting things done, you write a bunch of hacky code all over the place. Well for this refactoring, this was indeed the scanner. There was a bunch of duplication for reading into a slice of structs or reading into a single struct. All of this has been cleaned up and refactored into nice logical functions so you can see what's happening. As a result, we've been able to add a bunch of tests for all the edge cases encountered so far. This boosts the test coverage of the scanner which is great because it's one of the more fragile components with all the reflection it needs to do. I've also taken this opportunity to rename the ScanAll to ScanIter. My rationale for this is because ScanAll feels like it's going to scan everything in the iterator. This might not be the case all the time especially if you pass in a single struct where multiple results have been returned.
I've developed a benchmark which allows for similar comparisons between
the old method of decoding using mapstructure and the new decoder. There
is some differences due to the fact that we use a mock iterator which
will skew the results (in favour of mapstructure as that skips all the
MapScan business it would usually have to do) but the results show that
it's no contest anyway (our optimisations win by a country mile).
Using mapstructure:
goos: darwin
goarch: amd64
pkg: github.com/monzo/gocassa
BenchmarkDecodeBlogSliceNoBody-4 3000 480115 ns/op 73486 B/op 1687 allocs/op
BenchmarkDecodeBlogStruct-4 100 12456151 ns/op 375150 B/op 30385 allocs/op
BenchmarkDecodeBlogStructEmptyBody-4 100000 22711 ns/op 3489 B/op 68 allocs/op
BenchmarkDecodeAlphaSlice-4 1000 1472692 ns/op 306253 B/op 5007 allocs/op
BenchmarkDecodeAlphaStruct-4 20000 68542 ns/op 14407 B/op 174 allocs/op
PASS
ok github.com/monzo/gocassa 9.605s
Doing our own reflection:
goos: darwin
goarch: amd64
pkg: github.com/monzo/gocassa
BenchmarkDecodeBlogSliceNoBody-4 30000 36761 ns/op 21336 B/op 238 allocs/op
BenchmarkDecodeBlogStruct-4 300000 3730 ns/op 2840 B/op 35 allocs/op
BenchmarkDecodeBlogStructEmptyBody-4 500000 4269 ns/op 2840 B/op 35 allocs/op
BenchmarkDecodeAlphaSlice-4 10000 109677 ns/op 44128 B/op 815 allocs/op
BenchmarkDecodeAlphaStruct-4 200000 11790 ns/op 9315 B/op 97 allocs/op
PASS
ok github.com/monzo/gocassa 9.017s
Map types are a bit unique in the mock because in order to dynamically
do diff based upserts, the map is treated like an interface map value
type ( map[<KeyType>]interface{}) in certain cases. In order to support
this, we do some type assertions in the map case.
Also fixes a test in mock_test.go which we changed previously to
accomodate the refactor. This means we haven't had to change any of the
mock behaviour with this refactoring and all existing use cases work!
If a value hasn't been set previously and we try and select it, it might be the zero value and that'll cause a panic if we call Type() or similar on reflect.Zero. This commit adds a valid check to ensure we don't have the zero type (which is something we can just continue over).
To preserve old behaviour, if we are selecting a field and that field happens to be nil in our store, we're going to get a nil value back from gocassa. In this case, we are responsible for allocating the underlying value and setting it to a non-nil type.
We need to follow pointers for embedded types otherwise we won't correctly extract all the fields. Also, if we have embedded pointers which point to nil, we don't do anything about setting them.
sjwhitworth
left a comment
sjwhitworth
left a comment
There was a problem hiding this comment.
On the whole, it looks like a great improvement. The size of the PR makes it quite hard to review, but it's often hard to make these sorts of fundamental changes in small chunks. I left a few comments which would aid maintainability.
Some things that would make me more confident we hadn't changed behaviour:
- Get a corpus of query strings before and after your change: we shouldn't have changed any representation on the wire.
- Pick some extremely esoteric/weird behaviour of
gocassathat others invariably rely on and hand test this. You may have already done this, but I don't think I've got that level of context.
milesbxf
left a comment
milesbxf
left a comment
There was a problem hiding this comment.
A peripheral comment while I look at this in more detail, but have we considered using Go code generation to do this in future?
This would avoid needing any reflection at all, and be more type-safe
milesbxf
left a comment
milesbxf
left a comment
There was a problem hiding this comment.
I haven't had time to have the deep look this PR deserves, but I've added a few surface-level comments
| if timeline, ok := m["Timeline"]; ok { | ||
| if timeline.Name() != "Timeline" { | ||
| t.Errorf("Timeline should have name 'Timeline' but got %s", timeline.Name()) | ||
| } |
There was a problem hiding this comment.
I wonder if it's worth introducing an assertion library to make these less verbose (e.g. testify/assert) but appreciate we might not necessarily want to add a new dependency/way of testing things
There was a problem hiding this comment.
In fact I've seen that we're using testify/assert below in scanner_test.go, so we should almost definitely update these tests to be consistent
There was a problem hiding this comment.
I didn't want to drastically change this file and thus have decided to stick with the existing convention (especially because this is a different package too within gocassa). I don't think this needs to be a blocker
|
@milesbxf I did consider very heavily code generation but that got very complicated with the refactor. This doesn't stop it though so potentially as another update if needed it'd be great! @sjwhitworth FWIW this change hasn't changed any code paths around query generation. It's purely around query execution and reflection. I've added a heap of tests around based on mock and actual querying tests within our Monzo usage. The fact those tests pass and the necessary tests added here give the confidence needed. |
sjwhitworth
left a comment
sjwhitworth
left a comment
There was a problem hiding this comment.
Your changes look good. Would you mind if I took more time to take a final pass through? I think it would be good to get multiple eyes on this as well.
sjwhitworth
left a comment
sjwhitworth
left a comment
There was a problem hiding this comment.
Approved, with the caveat that I'd like another approver
| // setPtrs takes a list of fields and the associated pointers and sets them | ||
| // in order to the targetStruct | ||
| func setPtrs(structFields []*r.Field, ptrs []interface{}, targetStruct reflect.Value) { | ||
| for index, field := range structFields { |
There was a problem hiding this comment.
Potentially add a check to ensure structFields and ptrs are the same len
| var basePtr reflect.Value | ||
| switch baseType.Kind() { | ||
| case reflect.Array, reflect.Chan, reflect.Func, reflect.Interface, reflect.Ptr, reflect.UnsafePointer: | ||
| return fmt.Errorf("type of kind %v is not supported", baseType.Kind()) |
There was a problem hiding this comment.
Minor suggestion, might be worth explaining why in a comment (these aren't settable).
mattrco
left a comment
mattrco
left a comment
There was a problem hiding this comment.
Approving this on the basis that we've walked through the code and there are sufficient tests 👍
This PR replaces the use of the
mapstructurelibrary with custom reflection which is much more optimised forgocassainternals. Doing so leads to much more performant decoding characteristics.Motivation
The motivation for this PR came from profiling our services. Turns out quite a lot of services which use Cassandra at scale were spending 50-60%+ of their CPU within gocassa's
decodeResultstep (with a sizeable ~10% additionally spent inMapScan). This is a direct invocation ofmapstructure.Given I was going to tweak the interfaces, I took the liberty of cleaning up and adding a
Statementobject instead of ferrying around a query string and list of values. This can also encapsulate field name information under the hood which is used in our scanner.Change
This patch essentially hooks into the
gocqlScanner directly rather than going through a map forSELECTqueries.Previously we had two levels of reflection when reading via gocassa:
map[string]interface{}withingocql. This used theMapScanfunction ingocqland thus results in a map per result allocated and populateddecodeResultingocassato convert each map into the target structNow we have a direct reflection into struct by leveraging the ordering of fields which gocassa also knows about since gocassa is what constructs the
SELECTqueries in the first place!Benchmarks
gocassaonmaster(the benchmark test is at96f213d):These values are very high and it doesn't even account for the map allocation/population! I did some profiling and it turns out the
DecodeHookfunctionality we use to convert from thegocqltype of various integer types and also to supportUnmarshalCQLfunctionality adds a very large overhead! I got rid of them briefly and ran another test.gocassaonmasterwithout DecodeHook functions (the benchmark test is at96f213d):Still quite high but there has been noticeable improvement, especially in
BenchmarkDecodeBlogStructwhere decoding a single struct with a[]bytepopulated was making the per operation latency 9x slower. Note that we can't just get rid of these functions in real use because otherwise our integer types will be wrong. You don't even need to be using any of this functionality, just the conversion to execute these decode hooks adds a high amount of latency.gocassawith this PR on decoding into structs using the iterator directly:This is a 5-10x+ improvement in latency per operation and significantly reduced memory allocations by hooking into gocql directly. If you are using byte slices or other similar data structures within your target unmarshal struct, the improvement will be massive (upwards of hundreds of times faster).
This PR isn't adding any additional hidden work on the
gocqlside because this iteration cost is already being spent by usingMapScanwhich does the same work (in fact, as of writing this, it uses thegocql.Iter.Scan()under the hood).