Google-cloud-go: spanner/spansql: Minimize allocations in SQL()

[ericwenn]

We are using spansql.Query to SQL dynamic SQL queries, which has increased the readability of that code a lot which is great (compared to concatenating strings). However, after running some benchmarks on our services we noticed that the implementation of .SQL() accounts for 5% of the memory allocations (space) in our services.

Digging a bit deeper reveals it is due to the string concatenation happening within that method:

  Total:     99.03MB   109.03MB (flat, cum)  4.56%
    244            .          .             if sel.Distinct { 
    245            .          .                 str += "DISTINCT " 
    246            .          .             } 
    247            .          .             for i, e := range sel.List { 
    248            .          .                 if i > 0 { 
    249      35.01MB    35.01MB                     str += ", " 
    250            .          .                 } 
    251      48.51MB    57.01MB                 str += e.SQL() 
    252            .          .                 if len(sel.ListAliases) > 0 { 
    253            .          .                     alias := sel.ListAliases[i] 
    254            .          .                     if alias != "" { 
    255            .          .                         str += " AS " + ID(alias).SQL() 
    256            .          .                     } 
    257            .          .                 } 
    258            .          .             } 
    259            .          .             if len(sel.From) > 0 { 
    260          4MB        4MB                 str += " FROM " 
    261            .          .                 for i, f := range sel.From { 
    262            .          .                     if i > 0 { 
    263            .          .                         str += ", " 
    264            .          .                     } 
    265          5MB        5MB                     str += ID(f.Table).SQL() 
    266            .          .                 } 
    267            .          .             } 
    268            .          .             if sel.Where != nil { 
    269       6.50MB        8MB                 str += " WHERE " + sel.Where.SQL() 
    270            .          .             } 
    271            .          .             if len(sel.GroupBy) > 0 { 
    272            .          .                 str += " GROUP BY " 
    273            .          .                 for i, gb := range sel.GroupBy { 
    274            .          .                     if i > 0 {

It would be great to see the allocations within this method reduced!
An alternative we have considered is to memoize the result from SQL, but the impact of such an optimization is very dependent on how little or much variance the queries have.

[dsymonds]

spansql was never written with efficiency in mind, but it is pretty easy to improve the hot spots here. Let me do a quick pass, and see how it looks in your situation after that.

[ericwenn]

Totally understandable! Thanks for looking into this so swiftly, will give an update on this thread once the fix lands and I have tried it out. It looks promising!

[ericwenn]

Can confirm this removed 75% of allocations from .SQL(). Great job!

[dsymonds]

Cool. Can you identify any places in spansql that is still causing significant allocation for you? There's probably a few more easy wins that could be made.

[ericwenn]

The top contender (using HEAD) is the following:

cloud.google.com/go/spanner/spansql.ID.addSQL
/home/ericwenn/go/pkg/mod/cloud.google.com/go/[email protected]/spansql/sql.go

  Total:     21.52MB    25.02MB (flat, cum)  1.13%
    492            .          .           func (id ID) addSQL(sb *strings.Builder) { 
    493            .          .             // https://cloud.google.com/spanner/docs/lexical#identifiers 
    494            .          .            
    495            .          .             // TODO: If there are non-letters/numbers/underscores then this also needs quoting. 
    496            .          .            
    497            .     3.50MB             if IsKeyword(string(id)) { 
    498            .          .                 // TODO: Escaping may be needed here. 
    499            .          .                 sb.WriteString("`") 
    500            .          .                 sb.WriteString(string(id)) 
    501            .          .                 sb.WriteString("`") 
    502            .          .                 return 
    503            .          .             } 
    504            .          .            
    505      21.52MB    21.52MB             sb.WriteString(string(id)) 
    506            .          .           } 
    507            .          .            
    508            .          .           func (p Param) SQL() string { return buildSQL(p) } 
    509            .          .           func (p Param) addSQL(sb *strings.Builder) { 
    510            .          .             sb.WriteString("@") 

However, I'm not sure how much of a low-hanging fruit this is.

[dsymonds]

Yeah, I think the only potential is to pre-allocate or reuse strings.Builders, which would be too invasive I think. Let's call this done for now.

[ericwenn]

Agree, already great improvement. Thanks!