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

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

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

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

valyala opened this issue Apr 1, 2017 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Performance
Milestone

Comments

@valyala
Copy link
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
Copy link
Contributor Author

valyala commented Apr 1, 2017

Issues similar to #18625 could be resolved with this optimization

@randall77
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

valyala commented May 9, 2017

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

@bradfitz bradfitz added this to the Unplanned milestone May 19, 2017
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Performance
Projects
None yet
Development

No branches or pull requests

5 participants