Skip to content
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

runtime: don't allocate for non-escaping conversions to interface{} #8618

Open
randall77 opened this issue Aug 29, 2014 · 12 comments
Open

runtime: don't allocate for non-escaping conversions to interface{} #8618

randall77 opened this issue Aug 29, 2014 · 12 comments
Milestone

Comments

@randall77
Copy link
Contributor

randall77 commented Aug 29, 2014

fmt.Fprintf("%d", 8)

Since all interface data fields are now pointers, an int must be allocated and
initialized to 8 so that it can be put in an interface{} to pass to Fprintf.

Since we know the 8 doesn't escape, we could instead allocate that 8 on the stack and
have the interface data word point to that stack slot.  To be safe, we can only do this
when the resulting interface{} doesn't escape.  We probably also need to be sure the
conversion happens at most once so the stack slot is not reused.

We could have a special convT2Enoescape call for the compiler to use when it knows the
result doesn't escape.  Maybe also convT2I, assertT2E, ...
@robpike
Copy link
Contributor

robpike commented Aug 29, 2014

Comment 1:

Status changed to Accepted.

@griesemer
Copy link
Contributor

griesemer commented Oct 1, 2014

Comment 2:

Labels changed: added repo-main.

@jeffallen
Copy link
Contributor

jeffallen commented Dec 10, 2014

This thread explains how this issue causes 2 allocs on every call to os.(*File).Write.
https://groups.google.com/forum/#!topic/golang-nuts/0hfeLJP1LSk

@josharian
Copy link
Contributor

josharian commented Mar 23, 2015

Dmitry started this in CL 3503. Note that this requires improved escape analysis.

@gopherbot
Copy link

gopherbot commented Jan 24, 2017

CL https://golang.org/cl/35554 mentions this issue.

@navytux
Copy link
Contributor

navytux commented Apr 20, 2017

For the reference until this is fixed some of us use ad-hoc printf-style mini language to do text formatting in hot codepaths without allocations. For example if in fmt speak you have

s := fmt.Sprintf("hello %d %s %x", 1, "world", []byte("data"))

the analog would be

buf := xfmt.Buffer{}
buf .S("hello ") .D(1) .C(' ') .S("world") .C(' ') .Xb([]byte("data"))
s := buf.Bytes()

It is a bit uglier but runs faster and without allocations:

pkg: lab.nexedi.com/kirr/go123/xfmt
BenchmarkFmt-4       5000000           246 ns/op          72 B/op      3 allocs/op
BenchmarkXFmt-4     20000000            57.9 ns/op         0 B/op      0 allocs/op

Details:

https://lab.nexedi.com/kirr/go123/commit/1aa677c8
https://lab.nexedi.com/kirr/go123/blob/c0bbd06e/xfmt/fmt.go

@josharian
Copy link
Contributor

josharian commented Apr 20, 2017

Note that when the arguments are constants, they no longer allocate on tip, so this is a bit better than it was.

@navytux
Copy link
Contributor

navytux commented Apr 20, 2017

@josharian thanks for feedback. For the reference the above benchmark was for tip (go version devel +d728be70f4 Thu Apr 20 01:37:08 2017 +0000 linux/amd64).

@bnjjj
Copy link

bnjjj commented Nov 15, 2017

What are the status about this issue ? Is anyone working on ?

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Nov 15, 2017

@bnjjj I'm not aware of anybody actively working on this.

@rogpeppe
Copy link
Contributor

rogpeppe commented May 19, 2020

One data point here. The gio authors are considering an API design choice: whether to use an interface type or a function type. Using the interface type results in nicer code (when there's a literal struct, it can be used directly rather than via a method expression to obtain the function value), but results in allocations, and avoiding allocations is a key consideration in that context.

To illustrate with a couple of small examples:

This program, using a function type, does not incur any allocations. The required closures can all be allocated on the stack. https://play.golang.org/p/FC4taLlhVGz

This equivalent program, using an interface type, incurs an allocation for each interface conversion. https://play.golang.org/p/XPfcNSMPt82

Given that interfaces are often more idiomatic than function types in Go, it would be nice if they could be used interchangeably without a significant performance difference.

@josharian
Copy link
Contributor

josharian commented May 19, 2020

As of five years ago(!), this needed escape analysis improvements. Those stalled; see CL 3503. Maybe @mdempsky could weigh in.

fsmv added a commit to fsmv/in.go that referenced this issue Aug 29, 2021
This was quite a struggle to do in a nice way!

1. Due to golang/go#8618
   anything you pass to a fmt.* call gets copied to the heap. Eliminating
   that was simply replacing fmt.Fprintln with the bytes.Buffer calls and
   calling os.Stderr.Write directly.

2. Calling time.Duration.String() unfortunately forces a copy to the
   heap because it returns a string copy of its internal byte slice on
   the stack. So I had to reimplement that in a way that just writes to
   my buffer. The challenge was keeping the implementation under
   100 lines of code. To help with that I skipped the <1s printing case
   the standard lib has. Copying the official implementation and patching
   it would probably result in more efficient code but it is long.
HonoreDB added a commit to HonoreDB/cockroach that referenced this issue Sep 24, 2022
EncodeEscapedChar (which is called in EncodeSQLStringWithFlags)
is pretty optimized, but for escaping a multibyte character it
was using fmt.FPrintf, which means every multibyte character
ended up on the heap due to golang/go#8618.
This had a noticeable impact in changefeed benchmarking.

This commit just hand-compiles the two formatting strings that
were being used into reasonably efficient go, eliminating the allocs.

Benchmark encoding the first 10000 runes shows a 4x speedup:

Before: BenchmarkEncodeNonASCIISQLString-16    	     944	   1216130 ns/op
After: BenchmarkEncodeNonASCIISQLString-16    	    3468	    300777 ns/op

Release note: None
HonoreDB added a commit to HonoreDB/cockroach that referenced this issue Sep 27, 2022
EncodeEscapedChar (which is called in EncodeSQLStringWithFlags)
is pretty optimized, but for escaping a multibyte character it
was using fmt.FPrintf, which means every multibyte character
ended up on the heap due to golang/go#8618.
This had a noticeable impact in changefeed benchmarking.

This commit just hand-compiles the two formatting strings that
were being used into reasonably efficient go, eliminating the allocs.

Benchmark encoding the first 10000 runes shows a 4x speedup:

Before: BenchmarkEncodeNonASCIISQLString-16    	     944	   1216130 ns/op
After: BenchmarkEncodeNonASCIISQLString-16    	    3468	    300777 ns/op

Release note: None
craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Sep 30, 2022
86061: cdc, externalconn: enable external connections for all cdc sinks r=HonoreDB a=HonoreDB

This PR expands the kafka external connection sink to a generalized
external connection sink for all changefeed sink types. Because these
overlap with backup sink types, this necessitates changing the external
connection registration logic slightly: different consumers of the same scheme need to use the same parsing logic, but can have different validation logic.

Release justification: Required for use of external connections.

88671: util: avoid allocations when escaping multibyte characters r=[miretskiy,yuzefovich] a=HonoreDB

EncodeEscapedChar (which is called in EncodeSQLStringWithFlags) is pretty optimized, but for escaping a multibyte character it was using fmt.FPrintf, which means every multibyte character ended up on the heap due to golang/go#8618. This had a noticeable impact in changefeed benchmarking.

This commit just hand-compiles the two formatting strings that were being used into reasonably efficient go, eliminating the allocs.

Benchmark encoding the first 10000 runes shows a 4x speedup:

Before: BenchmarkEncodeNonASCIISQLString-16    	     944	   1216130 ns/op
After: BenchmarkEncodeNonASCIISQLString-16    	    3468	    300777 ns/op

Release note: None

Co-authored-by: Aaron Zinger <zinger@cockroachlabs.com>
craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Sep 30, 2022
88425: colexec: use tree.DNull when projection is called on null input r=DrewKimball a=DrewKimball

Most projections skip rows for which one or more arguments are null, and just output a null for those rows. However, some projections can actually accept null arguments. Previously, we were using the values from the vec even when the `Nulls` bitmap was set for that row, which invalidates the data in the vec for that row. This could cause a non-null value to be unexpectedly concatenated to an array when an argument was null (nothing should be added to the array in this case).

This commit modifies the projection operators that operate on datum-backed vectors to explicitly set the argument to `tree.DNull` in the case when the `Nulls` bitmap is set. This ensures that the projection is not performed with the invalid (and arbitrary) value in the datum vec at that index.

Fixes #87919

Release note (bug fix): Fixed a bug in `Concat` projection operators for arrays that could cause non-null values to be added to the array when one of the arguments was null.

88671: util: avoid allocations when escaping multibyte characters r=[miretskiy,yuzefovich] a=HonoreDB

EncodeEscapedChar (which is called in EncodeSQLStringWithFlags) is pretty optimized, but for escaping a multibyte character it was using fmt.FPrintf, which means every multibyte character ended up on the heap due to golang/go#8618. This had a noticeable impact in changefeed benchmarking.

This commit just hand-compiles the two formatting strings that were being used into reasonably efficient go, eliminating the allocs.

Benchmark encoding the first 10000 runes shows a 4x speedup:

Before: BenchmarkEncodeNonASCIISQLString-16    	     944	   1216130 ns/op
After: BenchmarkEncodeNonASCIISQLString-16    	    3468	    300777 ns/op

Release note: None

Co-authored-by: DrewKimball <drewk@cockroachlabs.com>
Co-authored-by: Aaron Zinger <zinger@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests