Reduce allocations for read path #166

berfarah · 2018-09-12T20:30:00Z

Two commits here.

First one reduces allocations by 4 because we're no longer copying references for new scanners/valuers.

Second one reduces allocations by not copying our value, but rather allocating straight to a struct.

berfarah · 2018-09-12T20:32:35Z

Note: I need to update some tests to pass. However, I'm not prioritizing this until I confirm on a staging server that this runs much more efficiently.

coveralls · 2018-09-12T21:23:07Z

Pull Request Test Coverage Report for Build 1017

75 of 87 (86.21%) changed or added relevant lines in 4 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.05%) to 64.555%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
sqlgen/reflect.go	38	50	76.0%

Files with Coverage Reduction	New Missed Lines	%
graphql/schemabuilder/reflect.go	4	71.81%

Totals
Change from base Build 970:	0.05%
Covered Lines:	3282
Relevant Lines:	5084

💛 - Coveralls

willhug · 2018-09-12T21:29:27Z

fields/sql.go

 	// Get a value of the pointer of our type. The Scanner and Unmarshalers should
 	// only be implemented as dereference methods, since they would do nothing otherwise. Therefore
 	// we can safely assume that we should check for these interfaces on the pointer value.
-	s.value = reflect.New(s.Type)
+	// s.value = s.va


willhug

Nice! Looks pretty decent, a couple questions about some edge cases, but I don't think they'll be issues.

willhug · 2018-09-12T21:32:48Z

fields/descriptor.go

@@ -39,7 +39,12 @@ func (d *Descriptor) Valuer(val reflect.Value) Valuer {
 }

 // Scanner creates a sql.Scanner from the descriptor.
-func (d *Descriptor) Scanner() *Scanner { return &Scanner{Descriptor: d} }
+func (d *Descriptor) Scanner(value reflect.Value) *Scanner {


value doesn't convey a lot of meaning, maybe dest?

This is gone now

willhug · 2018-09-13T18:48:48Z

fields/descriptor.go

@@ -39,7 +39,12 @@ func (d *Descriptor) Valuer(val reflect.Value) Valuer {
 }

 // Scanner creates a sql.Scanner from the descriptor.
-func (d *Descriptor) Scanner() *Scanner { return &Scanner{Descriptor: d} }
+func (d *Descriptor) Scanner(value reflect.Value) *Scanner {


Maybe add some more context on this commit as well.

AFAIK I removed this - but definitely

willhug · 2018-09-13T21:06:23Z

livesql/binlog.go

-		scanner := table.Columns[i].Descriptor.Scanner()
+		column := table.Columns[i]
+		field := elem.FieldByIndex(column.Index)
+		if field.Kind() != reflect.Ptr && field.CanAddr() {


this this alright when it's not a pointer and we can't Addr it? Or is that not physically possible?

It should not be possible. However, you're right - if it's not a pointer and we can't addr it, we should return an error here.

Hmm, actually, maybe I should remove field.CanAddr() and just have it panic if it can't. This should always be able to Addr - the struct is even defined in this method

changpingc

seems reasonable

changpingc · 2018-09-13T21:29:15Z

fields/descriptor.go

-	if !isValid {
-		to.Set(reflect.Zero(to.Type()))
-		return
+	// We need to hold onto this pointer-pointer in order to make the value addressable.


reflect law 3 https://blog.golang.org/laws-of-reflection 👍

changpingc · 2018-09-13T21:30:13Z

fields/sql.go

-// CopyTo copies the scanner value to another reflect.Value. This is used for setting structs.
-func (s *Scanner) CopyTo(to reflect.Value) {
-	s.copy(s.value, to, s.isValid)
+func (s *Scanner) To(value reflect.Value) {


Not sure if To name makes sense. Maybe Reset makes more sense?

One example I see often is having a sync.Pool of compression writers, like https://github.com/ungerik/go-pool/blob/master/gzip.go#L11:31. They almost always have a Reset method to replace underlying writer/destination.

I'm open to renaming this. Maybe it should be Target? Also, now that I think about it, maybe I should clear the value at the end of Scan.

berfarah · 2018-09-13T22:14:56Z

Benchmark before/after:

Original

goos: darwin
goarch: amd64
pkg: github.com/samsarahq/thunder/sqlgen
Benchmark/Read-8         	    5000	    358456 ns/op	    1766 B/op	      45 allocs/op
Benchmark/Create-8       	    2000	    677856 ns/op	    1104 B/op	      23 allocs/op
Benchmark/Update-8       	    2000	    682379 ns/op	    1488 B/op	      30 allocs/op
Benchmark/Delete-8       	    2000	    626050 ns/op	     592 B/op	      16 allocs/op

New Types

goos: darwin
goarch: amd64
pkg: github.com/samsarahq/thunder/sqlgen
Benchmark/Read-8         	    5000	    291292 ns/op	    2239 B/op	      60 allocs/op
Benchmark/Create-8       	    2000	    683282 ns/op	    1568 B/op	      39 allocs/op
Benchmark/Update-8       	    2000	    576854 ns/op	    1976 B/op	      49 allocs/op
Benchmark/Delete-8       	    2000	    643999 ns/op	    1000 B/op	      28 allocs/op

New Types Optimized

goos: darwin
goarch: amd64
pkg: github.com/samsarahq/thunder/sqlgen
Benchmark/Read-8         	    5000	    312561 ns/op	    1670 B/op	      43 allocs/op
Benchmark/Read_Where-8   	    2000	    606886 ns/op	    2348 B/op	      56 allocs/op
Benchmark/Create-8       	    2000	    529853 ns/op	    1120 B/op	      26 allocs/op
Benchmark/Update-8       	    3000	    509400 ns/op	    1496 B/op	      32 allocs/op
Benchmark/Delete-8       	    3000	    457721 ns/op	     808 B/op	      25 allocs/op

Improvements across the board 😄

By having an indirect Descriptor, we were copying it into Scanners and Valuers. This reduces the allocations by using the same pointer to a Descriptor everywhere. Benchmark: ``` goos: darwin goarch: amd64 pkg: github.com/samsarahq/thunder/sqlgen Benchmark/Read-8 5000 258781 ns/op 1994 B/op 52 allocs/op Benchmark/Read_Where-8 3000 522702 ns/op 2707 B/op 67 allocs/op Benchmark/Create-8 3000 527204 ns/op 1376 B/op 35 allocs/op Benchmark/Update-8 3000 549079 ns/op 1784 B/op 45 allocs/op Benchmark/Delete-8 2000 544365 ns/op 808 B/op 24 allocs/op ```

Remove the in-between allocation that we are making for every value on every struct (causing additional heap allocations) and instead scan directly into the struct. Benchmark: ``` goos: darwin goarch: amd64 pkg: github.com/samsarahq/thunder/sqlgen Benchmark/Read-8 3000 337823 ns/op 1908 B/op 47 allocs/op Benchmark/Read_Where-8 2000 728402 ns/op 2631 B/op 62 allocs/op Benchmark/Create-8 2000 585395 ns/op 1376 B/op 35 allocs/op Benchmark/Update-8 2000 710547 ns/op 1784 B/op 45 allocs/op Benchmark/Delete-8 2000 639660 ns/op 808 B/op 24 allocs/op ```

Re-use scanners across different runs in order to prevent more allocation than we need. There's a trade-off here: On one hand, you have a mutex lock that can slow down code. On the other hand, we have allocations that can increase memory. For our use-case (based on simulated traffic), there is no perceptible slowdown and a big memory win. Benchmark: ``` goos: darwin goarch: amd64 pkg: github.com/samsarahq/thunder/sqlgen Benchmark/Read-8 5000 267699 ns/op 1686 B/op 43 allocs/op Benchmark/Read_Where-8 3000 506274 ns/op 2394 B/op 58 allocs/op Benchmark/Create-8 3000 520147 ns/op 1376 B/op 35 allocs/op Benchmark/Update-8 3000 493141 ns/op 1784 B/op 45 allocs/op Benchmark/Delete-8 3000 491342 ns/op 808 B/op 24 allocs/op ```

willhug

One comment about a commit message. Also the benchmarks you posted aren't 1:1, should we have the read_where bench in the first one as well?

willhug · 2018-09-13T23:08:04Z

sqlgen/reflect.go

@@ -51,16 +51,20 @@ const (
 )

 // UnbuildStruct extracts SQL values from a struct
-func UnbuildStruct(table *Table, strct interface{}) []interface{} {
+func UnbuildStruct(table *Table, strct interface{}) ([]interface{}, error) {


from the commit message:

Don't hold onto valuers, instead only use them to get values back. By doing this, we're allocating the simpler (and more likely to be used) value onto the heap, while only allocating Valuer onto the heap.

do we mean:

Don't hold onto valuers, instead only use them to get values back. By doing this, we're allocating the simpler (and more likely to be used) value onto the heap, while only allocating Valuer onto the **stack**.

?

If not maybe we should reword this.

Don't hold onto valuers, instead only use them to get values back. By doing this, we're allocating the simpler (and more likely to be used) value onto the heap, while only allocating Valuer onto the stack. Benchmark: ``` goos: darwin goarch: amd64 pkg: github.com/samsarahq/thunder/sqlgen Benchmark/Read-8 5000 304845 ns/op 1683 B/op 43 allocs/op Benchmark/Read_Where-8 2000 589746 ns/op 2346 B/op 56 allocs/op Benchmark/Create-8 2000 654167 ns/op 1120 B/op 26 allocs/op Benchmark/Update-8 2000 711194 ns/op 1496 B/op 32 allocs/op Benchmark/Delete-8 2000 705401 ns/op 808 B/op 25 allocs/op ```

This is internal API and should not be relied on by anyone

stephen

originally, we thought that the benchmark didn't capture the large production difference we saw. what was the realization that changed our minds there?

also, is there some back of the envelope math for how the benchmark translates to real world memory usage? (i.e. accounts for the 3GB we saw in production)

stephen · 2018-09-18T21:34:39Z

fields/descriptor.go

@@ -54,7 +54,7 @@ func (d Descriptor) ValidateSQLType() error {
 	return d.Scanner().Scan(val)
 }

-func (d Descriptor) copy(from, to reflect.Value, isValid bool) {
+func (d *Descriptor) copy(from, to reflect.Value, isValid bool) {


the interesting take away for me here is that pointer usage wrt allocation/heap/stack usage is tricky.

in this case, not copying the struct/using a pointer is cheaper (because... it's already on the heap?)

My takeaway here is: everything that's passed into a function, including pointers, is pass by value in go. If we have Descriptor, we are copying a value (all struct fields) and then pointing to it. If we have *Descriptor, we are copying a pointer (just one thing pointing to the value). The second is lighter because we don't have to re-allocate the struct and its children, we just have to allocate the pointer.

stephen · 2018-09-18T22:08:29Z

sqlgen/reflect.go

 	elem := reflect.ValueOf(strct).Elem()
 	values := make([]interface{}, len(table.Columns))

 	for i, column := range table.Columns {
 		val := elem.FieldByIndex(column.Index)
-		values[i] = column.Descriptor.Valuer(val)
+		var err error
+		values[i], err = column.Descriptor.Valuer(val).Value()


my takeaway: escaping a value is much cheaper than escaping a big struct?

Both that and sql was going to call Value anyway before it sent the value to the sql driver, which means that the other value was going to be escaped either way - might as well do it as soon as possible so we don't allocate two different values

Add BuildStruct method back into sqlgen so we can parse binlog row event the same way we scan from MySQL driver. This was removed in #166.

berfarah requested review from changpingc and willhug September 12, 2018 20:30

berfarah force-pushed the bf/optimize branch from 1622e41 to 36caf67 Compare September 12, 2018 21:17

berfarah requested a review from jellevandenhooff September 12, 2018 21:18

willhug reviewed Sep 12, 2018

View reviewed changes

berfarah changed the base branch from master to bf/types-2.0 September 12, 2018 21:56

berfarah force-pushed the bf/optimize branch 2 times, most recently from 36c7a9d to e53078b Compare September 13, 2018 18:18

willhug approved these changes Sep 13, 2018

View reviewed changes

berfarah changed the base branch from bf/types-2.0 to master September 13, 2018 21:17

changpingc approved these changes Sep 13, 2018

View reviewed changes

berfarah added 4 commits September 13, 2018 15:19

go: add benchmark for where clause

49ade60

berfarah force-pushed the bf/optimize branch from e53078b to fb09f60 Compare September 13, 2018 22:43

willhug approved these changes Sep 13, 2018

View reviewed changes

berfarah added 3 commits September 13, 2018 16:12

go fields: move into internal

4bcf49d

This is internal API and should not be relied on by anyone

Changelog update for 0.4.0 release

dd7fa16

berfarah force-pushed the bf/optimize branch from fb09f60 to dd7fa16 Compare September 13, 2018 23:12

berfarah merged commit 7b4f7dd into master Sep 13, 2018

berfarah deleted the bf/optimize branch September 13, 2018 23:17

stephen reviewed Sep 18, 2018

View reviewed changes

changpingc added a commit that referenced this pull request Oct 2, 2018

changelog: add #166 dependency serialization

fac630d

changpingc added a commit that referenced this pull request Oct 3, 2018

changelog: add #166 dependency serialization

f33aeea

changpingc added a commit that referenced this pull request Nov 8, 2018

go sqlgen: add BuildStruct

171cc42

Add BuildStruct method back into sqlgen so we can parse binlog row event the same way we scan from MySQL driver. This was removed in #166.

changpingc mentioned this pull request Nov 8, 2018

go sqlgen: add BuildStruct #195

Merged

changpingc added a commit that referenced this pull request Nov 8, 2018

go sqlgen: add BuildStruct

164f0e0

Add BuildStruct method back into sqlgen so we can parse binlog row event the same way we scan from MySQL driver. This was removed in #166.

changpingc added a commit that referenced this pull request Nov 9, 2018

go sqlgen: add BuildStruct

36f2770

Add BuildStruct method back into sqlgen so we can parse binlog row event the same way we scan from MySQL driver. This was removed in #166.

changpingc added a commit that referenced this pull request Nov 9, 2018

go sqlgen: add BuildStruct

9d8b898

Add BuildStruct method back into sqlgen so we can parse binlog row event the same way we scan from MySQL driver. This was removed in #166.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce allocations for read path #166

Reduce allocations for read path #166

berfarah commented Sep 12, 2018 •

edited

Loading

berfarah commented Sep 12, 2018

coveralls commented Sep 12, 2018 •

edited

Loading

willhug Sep 12, 2018

berfarah Sep 12, 2018

willhug left a comment

willhug Sep 12, 2018

berfarah Sep 13, 2018

willhug Sep 13, 2018

berfarah Sep 13, 2018

willhug Sep 13, 2018

berfarah Sep 13, 2018

berfarah Sep 13, 2018

changpingc left a comment

changpingc Sep 13, 2018

changpingc Sep 13, 2018

berfarah Sep 13, 2018

berfarah commented Sep 13, 2018 •

edited

Loading

willhug left a comment

willhug Sep 13, 2018

willhug Sep 13, 2018

berfarah Sep 13, 2018

berfarah Sep 13, 2018

stephen left a comment

stephen Sep 18, 2018

berfarah Sep 18, 2018

stephen Sep 18, 2018

berfarah Sep 18, 2018

Reduce allocations for read path #166

Reduce allocations for read path #166

Conversation

berfarah commented Sep 12, 2018 • edited Loading

berfarah commented Sep 12, 2018

coveralls commented Sep 12, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1017

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willhug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changpingc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berfarah commented Sep 13, 2018 • edited Loading

willhug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berfarah commented Sep 12, 2018 •

edited

Loading

coveralls commented Sep 12, 2018 •

edited

Loading

berfarah commented Sep 13, 2018 •

edited

Loading