Skip to content

Commit

Permalink
cmd/compile: eliminate fmtmode and fmtpkgpfx globals
Browse files Browse the repository at this point in the history
The fmtmode and fmtpkgpfx globals stand in the
way of making the compiler more concurrent (golang#15756).
This CL removes them.

The natural way to eliminate a global is to explicitly
thread it as a parameter through all function calls.
However, most of the functions in gc/fmt.go
get called indirectly, by way of fmt format strings,
so there's nowhere natural to add a parameter.

Since there are only a few fmtmode modes,
use named types to distinguish between modes.
For example, fmtNodeErr, fmtNodeDbg, and fmtNodeTypeId
are all gc.Node, but they print in different modes.
Varying the type allows us to thread mode through fmt.
Handle fmtpkgpfx by converting it to a printing mode,
FTypeIdName, and using the same type-based approach.

To avoid a loss of readability and danger of bugs
from introducing conversions at all call sites,
instead add a helper that systematically modifies the args.

The only remaining gc/fmt.go global is dumpdepth.
Since that is used for debugging only,
it that can be handled with a global mutex,
or some similarly basic, if inefficient, protection.

Passes toolstash -cmp. No compiler performance impact.

For future reference, other options for threading state
that were considered and rejected:

* Wrapping values in structs, such as:

  type fmtNode struct {
  	n *Node
  	mode fmtMode
  }

  This reduces the proliferation of types, and supports
  easily adding extra local parameters.
  However, putting such a struct into an interface{} allocates.
  This is unacceptable in this particular area of code.

* Passing state via precision, such as:

  fmt.Fprintf("%*v", mode, n)

  where mode is the state encoded as an integer.
  This avoids extra allocations, but it is out of keeping
  with the intended semantics of precision, and is less readable.

* Modify the fmt package to support setting/getting context
  via fmt.State. Unavailable due to Go 1 compatibility,
  and probably the wrong solution anyway.

* Give up on package fmt. This would be a huge readability
  regression and cause high code churn.

* Attempt a de-novo rewrite that circumvents these problems.
  Too high a risk of bugs, with insufficient reward for the effort,
  particularly since long term plans call for elimination
  of gc.Node.


Change-Id: Iea2440d5a34a938e64273707de27e3a897cb41d1
Reviewed-on: https://go-review.googlesource.com/38147
Run-TryBot: Josh Bleecher Snyder <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
  • Loading branch information
josharian committed Mar 14, 2017
1 parent 88cf932 commit 723ba18
Show file tree
Hide file tree
Showing 2 changed files with 416 additions and 293 deletions.
1 change: 1 addition & 0 deletions src/cmd/compile/fmt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,7 @@ var knownFormats = map[string]string{
"cmd/compile/internal/gc.Val %#v": "",
"cmd/compile/internal/gc.Val %T": "",
"cmd/compile/internal/gc.Val %v": "",
"cmd/compile/internal/gc.fmtMode %d": "",
"cmd/compile/internal/gc.initKind %d": "",
"cmd/compile/internal/ssa.BranchPrediction %d": "",
"cmd/compile/internal/ssa.Edge %v": "",
Expand Down
Loading

0 comments on commit 723ba18

Please sign in to comment.