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

cmd/compile: avoid copying if copied value isn't modified #19817

Open
valyala opened this Issue Apr 1, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@valyala
Contributor

valyala commented Apr 1, 2017

Go 1.8 emits unnesessary value copyings in the following code (didn't test go tip yet, but it is likely it has the same behaviour):

func sum(p *[1000][1000]int) int {
  s := 0
  a := *p // copy a million of ints
  for i := range a { // possible copy
      b := a[i] // copy a thousand of ints
      for j := range b { // possible copy
          s += b[j]
      }
  }
  return s
}

All these copyings could be substituted by pointers, which would result in direct memory access pointed by p. This looks like safe optimization, since both a and b values are read-only in this code.
This optimization also could save stack space used for storing a and b values.

Someone could argue this optimization can't be applied since the memory pointed by p could be modified by concurrently running goroutines, which will result in data races. But this code is already vulnerable to the same data races when compiled without the optimization, since value copying isn't atomic.

@valyala

This comment has been minimized.

Contributor

valyala commented Apr 1, 2017

Issues similar to #18625 could be resolved with this optimization

@randall77

This comment has been minimized.

Contributor

randall77 commented Apr 1, 2017

A copy is required for the a := *p statement.
The reason is the same as for this code:

   var p *int = ...
   x := *p
   return x - x

We want this code to always return 0, even in the presence of data races.
(This isn't in the spec AFAICT but we have code that relies on it. I accidentally did this optimization before and it broke mutexes. See the assignment old := m.state in sync/mutex.go:86. We can't load m.state twice.)

The internal assignments, like b := a[i], could be optimized away.

@go101

This comment has been minimized.

go101 commented Apr 2, 2017

@valyala, you can optimize it manually:

func sum(p *[1000][1000]int) int {
  s := 0
  a := p // just a pointer copy
  for i := range a { // just a pointer copy
      b := &a[i] // just a pointer copy
      for j := range b { // just a pointer copy
	   s += b[j]
      }
  }
  return s
}

or, even simpler

func sum(p *[1000][1000]int) int {
  s := 0
  for i := range p {
      for _, x := range &p[i] {
	   s += x
      }
  }
  return s
}
@valyala

This comment has been minimized.

Contributor

valyala commented Apr 6, 2017

@go101 , programmers frequently forget about implicit copying and stack usage, which results to issues like #18625 . See also this tweet. So it would be great if compiler could assist the programmer in such cases.

@randall77 , probably the optimization could be applied only to arrays and structs?

@randall77

This comment has been minimized.

Contributor

randall77 commented Apr 6, 2017

That would help the mutex case. But I still don't think it is advisable.

var p *[10]int = ...
a := *p
println(a[5])
println(a[5])

It would be very surprising if it printed different values, even in the presence of data races.

@valyala

This comment has been minimized.

Contributor

valyala commented May 9, 2017

@bradfitz , could you add "performance" label to this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment