THRIFT-5767: Go Simple JSON Protocol re-allocates memory for every escaped quote

[k-walton]

The code in ParseQuotedStringBody() was recursing in our application every time there was an escaped quote in strings and generated a new string buffer for every recursive call. This was a real issue for escaped JSON in our application in particular.

So this pull requests moves that logic into a loop, and uses a string builder to avoid re-allocating buffers for every iteration.

We observed an ~40x decrease in p99 latency from 2 seconds -> 50 milliseconds on our servers when implementing this change. So it is a substantial performance improvement for this particular use case.

• Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit? (not required, but preferred)
• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

[fishy]

The code looks good to me, but some suggestions:

Can you please add a benchmark test with the example you gave in the jira ticket, and post the before/after result here, with b.ReportAllocs() called? since this benchmark test will require a thrift definition, you likely cannot have it under lib/go/thrift and will need it to be under lib/go/test instead.

Depending on the result, I would also suggest you to write an unit test (also likely need to be under lib/go/test, in the same file as the benchmark test), that runs the benchmark test (via https://pkg.go.dev/testing#Benchmark), and checks the result, and verify that AllocsPerOp is below a certain threshold.

[k-walton]

The code looks good to me, but some suggestions:

Can you please add a benchmark test with the example you gave in the jira ticket, and post the before/after result here, with b.ReportAllocs() called? since this benchmark test will require a thrift definition, you likely cannot have it under lib/go/thrift and will need it to be under lib/go/test instead.

Depending on the result, I would also suggest you to write an unit test (also likely need to be under lib/go/test, in the same file as the benchmark test), that runs the benchmark test (via https://pkg.go.dev/testing#Benchmark), and checks the result, and verify that AllocsPerOp is below a certain threshold.

Sure! I was trying to keep the change set small, but those sound like a great idea! I'll go ahead and take some time to add them.

[k-walton]

@fishy I've added the test to validate that the benchmark's memory allocation is below a threshold. I chose the AllocedBytesPerOp() stat to make a limit, due to the normal protocol buffer reads using allocations as well making the results less clear.

I'll add the before and after below.

Before change:

=== RUN   BenchmarkSimpleJsonStringParse_Allocations
BenchmarkSimpleJsonStringParse_Allocations
BenchmarkSimpleJsonStringParse_Allocations-10               5881            222369 ns/op         1073930 B/op       2016 allocs/op

After change:

=== RUN   BenchmarkSimpleJsonStringParse_Allocations
BenchmarkSimpleJsonStringParse_Allocations
BenchmarkSimpleJsonStringParse_Allocations-10              28100             41955 ns/op           15440 B/op       1027 allocs/op