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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: postpone argument conversions after inlining #20087

Open
rasky opened this issue Apr 23, 2017 · 4 comments
Open

cmd/compile: postpone argument conversions after inlining #20087

rasky opened this issue Apr 23, 2017 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Unplanned

Comments

@rasky
Copy link
Member

rasky commented Apr 23, 2017

Look at this code:

package main

import (
	"fmt"
)

type Log struct {
	Enabled bool
}

func (l *Log) Print(args ...interface{}) {
	if l.Enabled {
		fmt.Println(args...)
	}
}

func main() {
	var x int = 1000
	l := Log{Enabled: false}
	l.Print("data", x)
}

If the code is compiled with -gcflags=-l=4, Log.Print() is inlined into main(). Unfortunately, this does not get rid of the expensive argument conversions to empty interfaces.

I know some work has been merged to speed up conversions of common data to empty interfaces, and that's great for when you are actually logging; but if logging is disabled, you can't fully get rid of the overhead.

I have a real high-performance program where I selectively enable logging for debugging reasons. Unfortunately, even with logging disabled, the overhead is measurable so I'm forced to actually comment out logging lines.

If the above pattern was optimized correctly, I could reword the API of my high-perf logging library to make sure such an optimization triggers for me.
@rasky rasky changed the title cmd/compile: postpone argument conversions after inling cmd/compile: postpone argument conversions after inlining Apr 23, 2017
@rasky
Copy link
Member Author

rasky commented Apr 23, 2017

(the issue title sucks, I can't come up with a proper title, feel free to rename)

@dr2chase
Copy link
Contributor

If we could more aggressively sink code within the inlined "then" arm of the if statement, that would do most of what you want, right?

But wow, yuck, it's this self-referential mem-touching mess that SSA is not going to easily recognize is side-effect-free.

@cherrymui
Copy link
Member

There are actually a few things going on:

  1. The Print method has a pointer receiver, so it makes the compiler think l in main is address-taken, and therefore not SSA'd. As a consequence false is stored and the loaded later, not propagated as a constant false (because there are calls in between). The call to fmt.Println is not eliminated, neither are the arguments.

    • After inlining, l isn't necessarily address-taken though.

  2. (Even l is not address-taken) Variadic call uses array for the arguments. Array is not SSA'd. Arguments are stored to the array. So it doesn't know the arguments are actually not needed.

    • The compiler probably should think store to locals dead if it is not read by anything (calls are considered as read).
    • Even so, the call to convT2E64 is probably still not eliminated.

@rasky
Copy link
Member Author

rasky commented Apr 23, 2017

If we could more aggressively sink code within the inlined "then" arm of the if statement, that would do most of what you want, right?

Yes, sounds like that would work.

@cherrymui: looks like your analysis focused on seeing why the code doesn't get turned into a big nop because of the constant false. This is interesting and could be a different bug, but it's not useful to me: in my real-world case, that boolean flag is not constant. What I'm interested into is how much useless code is being executed even if the variable is false (at runtime).

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Apr 13, 2018
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 13, 2018
@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
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@josharian @rasky @dr2chase @ianlancetaylor @gopherbot @cherrymui