x.go:4:3: syntax error: unexpected semicolon, expecting expression
x.go:4:4: non-name *%!v(PANIC=runtime error: invalid memory address or nil pointer dereference) on left side of :=
x.go:5:1: syntax error: unexpected }, expecting expression
After:
x.go:4:3: syntax error: unexpected semicolon, expecting expression
x.go:4:4: non-name *<N> on left side of :=
x.go:5:1: syntax error: unexpected }, expecting expression
No test because:
(1) we don't have a good mechanism to check for the
absence of the string "PANIC" in an error message
(2) the string "*<N>", while better, is itself ugly enough
that I don't want to actively check for it
(3) the bug isn't very important, the kind of thing only fuzzers encounter
(4) the fix is obvious and trivial
Filip Gruszczyński [Sun, 9 Apr 2017 17:44:39 +0000 (10:44 -0700)]
encoding/gob: use MakeMapWithSize when decoding map
This allows to pre-allocate the final size of the hashmap and avoid
re-allocating as we insert entries. Furthermore for the current
implementation of the hashmap it allows avoiding several rounds of
evacuating hashmap entries after each re-allocation.
Filip Gruszczynski [Sat, 15 Apr 2017 22:17:29 +0000 (15:17 -0700)]
runtime: don't panic for bad size hint in hashmap
Because the hint parameter is supposed to be treated
purely as a hint, if it doesn't meet the requirements
we disregard it and continue as if there was no hint
at all.
Alberto Donizetti [Tue, 2 May 2017 10:38:59 +0000 (12:38 +0200)]
doc/contribute: directly link to the Agreements page
There's no Settings->Agreement path for PolyGerrit users, but if we
link directly to the page in the instructions, Gerrit will inform them
that they can access the page by switching to the old UI.
Michael Munday [Sun, 30 Apr 2017 18:25:57 +0000 (14:25 -0400)]
cmd/{asm,compile}: avoid zeroAuto clobbering flags on s390x
This CL modifies how MOV[DWHB] instructions that store a constant to
memory are assembled to avoid them clobbering the condition code
(flags). It also modifies zeroAuto to use MOVD instructions instead of
CLEAR (which is assembled as XC).
MOV[DWHB]storeconst ops also no longer clobbers flags.
Note: this CL modifies the assembler so that it can no longer handle
immediates outside the range of an int16 or offsets from SB, which
reflects what the machine instructions support. The compiler doesn't
need this capability any more and I don't think this affects any existing
assembly, but it is easy to workaround if it does.
Fixes #20187.
Change-Id: Ie54947ff38367bd6a19962bf1a6d0296a4accffb
Reviewed-on: https://go-review.googlesource.com/42179 Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
5 shards, each of which spins up NumCPU processes,
each of which is running at GOMAXPROCS=NumCPU,
is too much for one machine. It makes my laptop unusable.
It might also be in part responsible for test flakes
that require a moderately responsive system,
like #18589 (backedge scheduling) and #19276 (locklinear).
It's possible that Go should be a better neighbor in general;
that's #17969. In the meantime, fix this corner of the world.
Builders snapshot the world and run shards on different
machines, so keeping sharding high for them is good.
cmd/internal/obj: fix LSym.Type during compilation, not linking
Prior to this CL, the compiler and assembler
were sloppy about the LSym.Type for LSyms
containing static data.
The linker then fixed this up, converting
Sxxx and SBSS to SDATA, and SNOPTRBSS to SNOPTRDATA
if it noticed that the symbol had associated data.
It is preferable to just get this right in cmd/compile
and cmd/asm, because it removes an unnecessary traversal
of the symbol table from the linker (see #14624).
Do this by touching up the LSym.Type fixes in
LSym.prepwrite and Link.Globl.
I have confirmed by instrumenting the linker
that the now-eliminated code paths were unreached.
And an additional check in the object file writing code
will help preserve that invariant.
There was a case in the Windows linker,
with internal linking and cgo,
where we were generating SNOPTRBSS symbols with data.
For now, convert those at the site at which they occur
into SNOPTRDATA, just like they were.
Does not pass toolstash-check,
but does generate identical linked binaries.
Elias Naur [Mon, 1 May 2017 18:35:08 +0000 (20:35 +0200)]
misc/android: don't let the Android exec wrapper hang indefinitely
On Android, the exec wrapper passes on output from adb to its parent
process by passing on os.Stderr and os.Stdout to adb. If the adb
process somehow hangs, it will keep stderr and stdout will open, in turn
blocking go test from ever returning from its cmd.Wait() even though
it has killed the exec wrapper process.
Break the short circuit by introducing a wrapper between adb and the
exec wrapper, preventing os/exec.Run from passing along the raw
file descriptors for os.Stdout and os.Stderr.
(Hopefully) fixes occasional indefinite hangs on the Android builder.
Michael Hudson-Doyle [Wed, 26 Apr 2017 23:56:42 +0000 (11:56 +1200)]
cmd/internal/obj/x86: use LEAx rather than ADDx when calling DUFFxxxx via GOT
DUFFZERO on 386 is not marked as clobbering flags, but rewriteToUseGot rewrote
"ADUFFZERO $offset" to "MOVL runtime.duffxxx@GOT, CX; ADDL $offset, CX; CALL CX"
which does. Luckily the fix is easier than figuring out what the problem was:
replace the ADDL $offset, CX with LEAL $offset(CX), CX.
On amd64 DUFFZERO clobbers flags, on arm, arm64 and ppc64 ADD does not clobber
flags and s390x does not use the duff functions, so I'm fairly confident this
is the only fix required.
I don't know how to write a test though.
Change-Id: I69b0958f5f45771d61db5f5ecb4ded94e8960d4d
Reviewed-on: https://go-review.googlesource.com/41821
Run-TryBot: Michael Hudson-Doyle <michael.hudson@canonical.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
cmd/compile: avoid giant init functions due to many user inits
We generate code that calls each user init function one at a time.
When there are lots of user init functions,
usually due to generated code, like test/rotate* or
github.com/juju/govmomi/vim25/types,
we can end up with a giant function,
which can be slow to compile.
This CL puts in an escape valve.
When there are more than 500 functions, instead of doing:
init.0()
init.1()
// ...
we construct a static array of functions:
var fns = [...]func(){init.0, init.1, ... }
and call them in a loop.
This generates marginally bigger, marginally worse code,
so we restrict it to cases in which it might start to matter.
500 was selected as a mostly arbitrary threshold for "lots".
Each call uses two Progs, one for PCDATA and one for the call,
so at 500 calls we use ~1000 Progs.
At concurrency==8, we get a Prog cache of about
1000 Progs per worker.
So a threshold of 500 should more or less avoid
exhausting the Prog cache in most cases.
Change-Id: I276b887173ddbf65b2164ec9f9b5eb04d8c753c2
Reviewed-on: https://go-review.googlesource.com/41500 Reviewed-by: Keith Randall <khr@golang.org>
overLoadFactor used a uintptr for its calculations.
When the number of potential buckets was large,
perhaps due to a coding error or corrupt/malicious user input
leading to a very large map size hint,
this led to overflow on 32 bit systems.
This overflow resulted in an infinite loop.
cmd/compile: add minor bit twiddling optimizations
Noticed while adding to the bitset implementation
in cmd/compile/internal/gc.
The (Com (Const)) optimizations were already present
in the AMD64 lowered optimizations.
They trigger 118, 44, 262, and 108 times
respectively for int sizes 8, 16, 32, and 64
in a run of make.bash.
The (Or (And)) optimization is new.
It triggers 3 times for int size 8
and once for int size 64 during make.bash,
in packages internal/poll, reflect,
encoding/asn1, and go/types,
so there is a bit of natural test coverage.
Michael Hudson-Doyle [Fri, 28 Apr 2017 00:22:50 +0000 (12:22 +1200)]
cmd/link: rename AttrHidden to AttrNotInSymbolTable
I want to move the SHIDDEN type bit into Attribute, but AttrHidden is already
there and means something completely different, so rename it. (I'll give the
SHIDDEN bit a better name when it moves too).
Bryan C. Mills [Thu, 16 Feb 2017 22:33:08 +0000 (17:33 -0500)]
archive/zip: replace RWMutex with sync.Map
This change replaces the compressors and decompressors maps with
instances of sync.Map, eliminating the need for Mutex locking in
NewReader and NewWriter.
The impact for encoding large payloads is miniscule, but as the
payload size decreases, the reduction in setup costs becomes
measurable.
Austin Clements [Tue, 21 Mar 2017 20:45:12 +0000 (16:45 -0400)]
runtime: reduce Windows timer resolution when idle
Currently Go sets the system-wide timer resolution to 1ms the whole
time it's running. This has negative affects on system performance and
power consumption. Unfortunately, simply reducing the timer resolution
to the default 15ms interferes with several sleeps in the runtime
itself, including sysmon's ability to interrupt goroutines.
This commit takes a hybrid approach: it only reduces the timer
resolution when the Go process is entirely idle. When the process is
idle, nothing needs a high resolution timer. When the process is
non-idle, it's already consuming CPU so it doesn't really matter if
the OS also takes timer interrupts more frequently.
Ben Shi [Tue, 25 Apr 2017 10:53:10 +0000 (10:53 +0000)]
cmd/compile/internal/ssa: more constant folding rules for ARM
(ADDconst [c] x) && !isARMImmRot(uint32(c)) && isARMImmRot(uint32(-c)) -> (SUBconst [int64(int32(-c))] x)
(SUBconst [c] x) && !isARMImmRot(uint32(c)) && isARMImmRot(uint32(-c)) -> (ADDconst [int64(int32(-c))] x)
Currently
a = a + 0xfffffff1 is compiled to （variable a is in R0)
MVN $14, R11
ADD R11, R0, R0
After applying the above 2 rules, it becomes
SUB $15, R0, R0
(BICconst [c] (BICconst [d] x)) -> (BICconst [int64(int32(c|d))] x)
This rule also optimizes the generated ARM code.
The other rules are added to avoid to generate less optimized ARM code
when substitutions ADD->SUB happen.
Change-Id: I3ead9aae2b446b674e2ab42d37259d38ceb93a4d
Reviewed-on: https://go-review.googlesource.com/41679 Reviewed-by: Keith Randall <khr@golang.org>
Instead of playing whack-a-mole finding all
the non-dowidth'd expressions that can sneak
out of the frontend and then deciding on
just the right place to handle them,
use a big hammer.
runtime/pprof: use symbol information already in profile in tests
Currently the pprof tests re-symbolize PCs in profiles, and do so in a
way that can't handle inlining. Proto profiles already contain full
symbol information, so this modifies the tests to use the symbol
information already present in the profile.
Daniel Theophanes [Fri, 28 Apr 2017 21:24:31 +0000 (14:24 -0700)]
database/sql: ensure releaseConn is defined before a possible close
When running a Query on Stmt a dependency is added to the stmt and
rows. To do that it needs a reference to Rows, so the releaseConn
function is defined after the definition. However the
rows.initContextClose was set to run before the releaseConn was
set on rows, setting up a situation where the connection could
be canceled before the releaseConn was set and resulting in
a segfault.
Austin Clements [Fri, 17 Jun 2016 13:33:33 +0000 (09:33 -0400)]
runtime: make _TinySizeClass an int8 to prevent use as spanClass
Currently _TinySizeClass is untyped, which means it can accidentally
be used as a spanClass (not that I would know this from experience or
anything). Make it an int8 to avoid this mix up.
This is a cherry-pick of dev.garbage commit 81b74bf9c5.
Austin Clements [Thu, 2 Jun 2016 15:09:20 +0000 (11:09 -0400)]
runtime: eliminate heapBitsSetTypeNoScan
It's no longer necessary to maintain the bitmap of noscan objects
since we now use the span metadata to determine that they're noscan
instead of the bitmap.
The combined effect of segregating noscan spans and the follow-on
optimizations is roughly a 1% improvement in performance across the
go1 benchmarks and the x/benchmarks, with no increase in heap size.
Austin Clements [Tue, 9 Feb 2016 22:53:07 +0000 (17:53 -0500)]
runtime: separate spans of noscan objects
Currently, we mix objects with pointers and objects without pointers
("noscan" objects) together in memory. As a result, for every object
we grey, we have to check that object's heap bits to find out if it's
noscan, which adds to the per-object cost of GC. This also hurts the
TLB footprint of the garbage collector because it decreases the
density of scannable objects at the page level.
This commit improves the situation by using separate spans for noscan
objects. This will allow a much simpler noscan check (in a follow up
CL), eliminate the need to clear the bitmap of noscan objects (in a
follow up CL), and improves TLB footprint by increasing the density of
scannable objects.
This is also a step toward eliminating dead bits, since the current
noscan check depends on checking the dead bit of the first word.
This has no effect on the heap size of the garbage benchmark.
We'll measure the performance change of this after the follow-up
optimizations.
This is a cherry-pick from dev.garbage commit d491e550c3. The only
non-trivial merge conflict was in updatememstats in mstats.go, where
we now have to separate the per-spanclass stats from the per-sizeclass
stats.
In particular, this says that Frames.Function uniquely identifies a
function within a program. We depend on this in various places that
use runtime.Frames in std, but it wasn't actually written down.
Change-Id: Ie7ede348c17673e11ae513a094862b60c506abc5
Reviewed-on: https://go-review.googlesource.com/41610 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Bryan C. Mills [Thu, 16 Feb 2017 22:44:16 +0000 (17:44 -0500)]
expvar: replace RWMutex usage with sync.Map and atomics
Int and Float already used atomics.
When many goroutines on many CPUs concurrently update a StringSet or a
Map with different keys per goroutine, this change results in dramatic
steady-state speedups.
This change does add some overhead for single-CPU and ephemeral maps.
I believe that is mostly due to an increase in allocations per call
(to pack the map keys and values into interface{} values that may
escape into the heap). With better inlining and/or escape analysis,
the single-CPU penalty may decline somewhat.
There are still two RWMutexes in the package: one for the keys in the
global "vars" map, and one for the keys in individual Map variables.
Those RWMutexes could also be eliminated, but avoiding excessive
allocations when adding new keys would require care. The remaining
RWMutexes are only acquired in Do functions, which I believe are not
typically on the fast path.
cmd/compile: check width of embedded interfaces in expandiface
The code in #20162 contains an embedded interface.
It didn't get dowidth'd by the frontend,
and during DWARF generation, ngotype asked
for a string description of it,
which triggered a request for the number of fields
in the interface, which triggered a dowidth,
which is disallowed in the backend.
The other changes in this CL are to support the test.
cmd/compile: use a map to track liveness variable indices
It is not safe to modify Node.Opt in the backend.
Instead of using Node.Opt to store liveness variable indices, use a map.
This simplifies the code and makes it much more clearly race-free.
There are generally few such variables, so the maps are not a significant
source of allocations; this also remove some allocations from putting
int32s into interfaces.
Because map lookups are more expensive than interface value extraction,
reorder valueEffects to do the map lookup last.
The only remaining use of Node.Opt is now in esc.go.
net/http: re-simplify HTTP/1.x status line writing
It used to be simple, and then it got complicated for speed (to reduce
allocations, mostly), but that involved a mutex and hurt multi-core
performance, contending on the mutex.
A change was sent to try to improve that mutex contention in
https://go-review.googlesource.com/c/42110/2/src/net/http/server.go
but that introduced its own allocations (the string->interface{}
boxing for the sync.Map key), which runs counter to the whole point of
that statusLine function: to remove allocations.
Instead, make the code simple again and not have a mutex. It's a bit
slower for the single-core case, but nobody with a single-user HTTP
server cares about 50 nanoseconds:
name old alloc/op new alloc/op delta
ResponseStatusLine 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal)
ResponseStatusLine-2 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal)
ResponseStatusLine-4 0.00B ±NaN% 0.00B ±NaN% ~ (all samples are equal)
name old allocs/op new allocs/op delta
ResponseStatusLine 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal)
ResponseStatusLine-2 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal)
ResponseStatusLine-4 0.00 ±NaN% 0.00 ±NaN% ~ (all samples are equal)
(Note the code could be even simpler with fmt.Fprintf, but that is
relatively slow and involves a bunch of allocations getting arguments
into interface{} for the call)
Daniel Martí [Thu, 27 Apr 2017 17:25:43 +0000 (18:25 +0100)]
cmd/go: error on space-separated list with comma
Using 'go build -tags "foo,bar"' might seem to work when you wanted
-tags "foo bar", since they make up a single tag that doesn't exist and
the build is unaffected.
Instead, error on any tag that contains a comma.
Fixes #18800.
Change-Id: I6641e03e2ae121c8878d6301c4311aef97026b73
Reviewed-on: https://go-review.googlesource.com/41951
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
os/exec: document that non-comparable writers may race
The comment for Cmd.Stdout and Cmd.Stderr says that it's safe to
set both to the same writer, but it doesn't say that this only
works when both writers are comparable.
This change updates the comment to explain that using a
non-comparable writer may still lead to a race.
Fixes #19804
Change-Id: I63b420034666209a2b6fab48b9047c9d07b825e2
Reviewed-on: https://go-review.googlesource.com/42052 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Michael Matloob [Wed, 5 Apr 2017 17:50:52 +0000 (13:50 -0400)]
runtime/pprof: propagate profile labels into profile proto
Profile labels added by the user using pprof.Do, if present will
be in a *labelMap stored in the unsafe.Pointer 'tag' field of
the profile map entry. This change extracts the labels from the tag
field and writes them to the profile proto.
runtime: fix profile handling of labels for race detector
If g1 sets its labels and then they are copied into a profile buffer
and then g2 reads the profile buffer and inspects the labels,
the race detector must understand that g1's recording of the labels
happens before g2's use of the labels. Make that so.
Dmitri Shuralyov [Wed, 26 Apr 2017 23:07:15 +0000 (19:07 -0400)]
cmd/go/internal/get: allow go get on github.com/ import paths with Unicode letters
More specifically, allow Unicode letters in the directories of GitHub
repositories, which can occur and don't have a valid reason to be
disallowed by go get.
Do so by using a predefined character class, the Unicode character
property class \p{L} that describes the Unicode characters that are
letters:
Since it's not possible to create GitHub usernames or repositories
containing Unicode letters at this time, those parts of the import path
are still restricted to ASCII letters only.
Kevin Burke [Mon, 24 Apr 2017 05:19:35 +0000 (22:19 -0700)]
regexp: speed up QuoteMeta with a lookup table
This is the same technique used in CL 24466. By adding a little bit of
size to the binary, we can remove a function call and gain a lot of
performance.
A raw array ([128]bool) would be faster, but is also be 128 bytes
instead of 16.
Running tip on a Mac:
name old time/op new time/op delta
QuoteMetaAll-4 192ns ±12% 120ns ±11% -37.27% (p=0.000 n=10+10)
QuoteMetaNone-4 186ns ± 6% 64ns ± 6% -65.52% (p=0.000 n=10+10)
name old speed new speed delta
QuoteMetaAll-4 73.2MB/s ±11% 116.6MB/s ±10% +59.21% (p=0.000 n=10+10)
QuoteMetaNone-4 139MB/s ± 6% 405MB/s ± 6% +190.74% (p=0.000 n=10+10)
Node.Used was written to from the backend
concurrently with reads of Node.Class
for the same ONAME Nodes.
I do not know why it was not failing consistently
under the race detector, but it is a race.
This is likely also a problem with Node.HasVal and Node.HasOpt.
They will be handled in a separate CL.
Fix Used by moving it to gc.Name and making it a separate bool.
There was one non-Name use of Used, marking OLABELs as used.
That is no longer needed, now that goto and label checking
happens early in the front end.
Leave the getters and setters in place,
to ease changing the representation in the future
(or changing to an interface!).
cmd/compile: add Type.MustSize and Type.MustAlignment
Type.Size and Type.Alignment are for the front end:
They calculate size and alignment if needed.
Type.MustSize and Type.MustAlignment are for the back end:
They call Fatal if size and alignment are not already calculated.
Most uses are of MustSize and MustAlignment,
but that's because the back end is newer,
and this API was added to support it.
This CL was mostly generated with sed and selective reversion.
The only mildly interesting bit is the change of the ssa.Type interface
and the supporting ssa dummy types.
Michael Hudson-Doyle [Wed, 19 Apr 2017 03:15:35 +0000 (15:15 +1200)]
cmd/internal/objabi, cmd/link: move linker-only symkind values into linker
Many (most!) of the values of objapi.SymKind are used only in the linker, so
this creates a separate cmd/link/internal/ld.SymKind type, removes most values
from SymKind and maps one to the other when reading object files in the linker.
Two of the remaining objapi.SymKind values are only checked for, never set and
so will never be actually found but I wanted to keep this to the most
mechanical change possible.
Change-Id: I4bbc5aed6713cab3e8de732e6e288eb77be0474c
Reviewed-on: https://go-review.googlesource.com/40985
Run-TryBot: Michael Hudson-Doyle <michael.hudson@canonical.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Hana Kim [Wed, 26 Apr 2017 21:58:31 +0000 (17:58 -0400)]
dwarf: add marker for embedded fields in dwarf
Currently, the following two codes generate the identical dwarf info
for type Foo.
prog 1)
type Foo struct {
Bar
}
prog 2)
type Foo struct {
Bar Bar
}
This change adds a go-specific attribute DW_AT_go_embedded_field
to annotate each member entry. Its absence or false value indicates
the corresponding member is not an embedded field.
Update #20037
Change-Id: Ibcbd2714f3e4d97c7b523d7398f29ab2301cc897
Reviewed-on: https://go-review.googlesource.com/41873 Reviewed-by: David Chase <drchase@google.com>
cmd/compile: randomize compilation order when race-enabled
There's been one failure on the race builder so far,
before we started sorting functions by length.
The race detector can only detect actual races,
and ordering functions by length might reduce the odds
of catching some kinds of races. Give it more to chew on.
Previously, the package did not distinguish between baseline and
extended sequential images. Both are non-progressive images, but the Th
range differs between the two, as per Annex B of
https://www.w3.org/Graphics/JPEG/itu-t81.pdf
Extended sequential images are often emitted by the Guetzli encoder.
Fixes #19913
Change-Id: I3d0f9e16d5d374ee1c65e3a8fb87519de61cff94
Reviewed-on: https://go-review.googlesource.com/41831 Reviewed-by: David Symonds <dsymonds@golang.org>
When using a concurrent backend,
the overall compilation time is bounded
in part by the slowest function to compile.
The number of top-level statements in a function
is an easily calculated and fairly reliable
proxy for compilation time.
Here's a standard compilecmp output for -c=8 with this CL:
A more interesting way of viewing the data is by
looking at the ratio of the time taken to compile
the slowest-to-compile function to the overall
time spent compiling functions.
If this ratio is small (near 0), then increased concurrency might help.
If this ratio is big (near 1), then we're bounded by that single function.
I instrumented the compiler to emit this ratio per-package,
ran 'go build -a -gcflags=-c=C -p=P std cmd' three times,
for varying values of C and P,
and collected the ratios encountered into an ASCII histogram.
Here's c=1 p=1, which is a non-concurrent backend, single process at a time:
The x-axis is floor(10*ratio), so the first column indicates the percent of
ratios that fell in the 0% to 9.9999% range.
We can see in this histogram that more concurrency will help;
in most cases, the ratio is small.
The sorting pays off; we are bound by the
compilation time of a single function in over half of packages.
The single * in the histogram indicates 0-10%.
The actual values for this chart are:
0: 5%, 1: 1%, 2: 1%, 3: 4%, 4: 5%, 5: 7%, 6: 7%, 7: 7%, 8: 9%, 9: 55%
This indicates that efforts to increase or enable more concurrency,
e.g. by optimizing mutexes or increasing the value of c,
will probably not yield fruit.
That matches what compilecmp tells us.
Further optimization efforts should thus focus instead on one of:
(1) making more functions compile concurrently
(2) improving the compilation time of the slowest functions
(3) speeding up the remaining serial parts of the compiler
(4) automatically splitting up some large autogenerated functions
into small ones, as discussed in #19751
I hope to spend more time on (1) before the freeze.
Adding process parallelism doesn't change the story much.
For example, here's c=8 p=8, after this CL:
Since we don't need to worry much about p,
these histograms can help us select a good
general value of c to use as a default,
assuming we're not bounded by GOMAXPROCS.
Here are some charts after this CL, for c from 1 to 8:
cmd/compile: add initial backend concurrency support
This CL adds initial support for concurrent backend compilation.
BACKGROUND
The compiler currently consists (very roughly) of the following phases:
1. Initialization.
2. Lexing and parsing into the cmd/compile/internal/syntax AST.
3. Translation into the cmd/compile/internal/gc AST.
4. Some gc AST passes: typechecking, escape analysis, inlining,
closure handling, expression evaluation ordering (order.go),
and some lowering and optimization (walk.go).
5. Translation into the cmd/compile/internal/ssa SSA form.
6. Optimization and lowering of SSA form.
7. Translation from SSA form to assembler instructions.
8. Translation from assembler instructions to machine code.
9. Writing lots of output: machine code, DWARF symbols,
type and reflection info, export data.
Phase 2 was already concurrent as of Go 1.8.
Phase 3 is planned for eventual removal;
we hope to go straight from syntax AST to SSA.
Phases 5–8 are per-function; this CL adds support for
processing multiple functions concurrently.
The slowest phases in the compiler are 5 and 6,
so this offers the opportunity for some good speed-ups.
Unfortunately, it's not quite that straightforward.
In the current compiler, the latter parts of phase 4
(order, walk) are done function-at-a-time as needed.
Making order and walk concurrency-safe proved hard,
and they're not particularly slow, so there wasn't much reward.
To enable phases 5–8 to be done concurrently,
when concurrent backend compilation is requested,
we complete phase 4 for all functions
before starting later phases for any functions.
Also, in reality, we automatically generate new
functions in phase 9, such as method wrappers
and equality and has routines.
Those new functions then go through phases 4–8.
This CL disables concurrent backend compilation
after the first, big, user-provided batch of
functions has been compiled.
This is done to keep things simple,
and because the autogenerated functions
tend to be small, few, simple, and fast to compile.
USAGE
Concurrent backend compilation still defaults to off.
To set the number of functions that may be backend-compiled
concurrently, use the compiler flag -c.
In future work, cmd/go will automatically set -c.
Furthermore, this CL has been intentionally written
so that the c=1 path has no backend concurrency whatsoever,
not even spawning any goroutines.
This helps ensure that, should problems arise
late in the development cycle,
we can simply have cmd/go set c=1 always,
and revert to the original compiler behavior.
MUTEXES
Most of the work required to make concurrent backend
compilation safe has occurred over the past month.
This CL adds a handful of mutexes to get the rest of the way there;
they are the mutexes that I didn't see a clean way to avoid.
Some of them may still be eliminable in future work.
In no particular order:
* gc.funcsymsmu. The global funcsyms slice is populated
lazily when we need function symbols for closures.
This occurs during gc AST to SSA translation.
The function funcsym also does a package lookup,
which is a source of races on types.Pkg.Syms;
funcsymsmu also covers that package lookup.
This mutex is low priority: it adds a single global,
it is in an infrequently used code path, and it is low contention.
Since funcsyms may now be added in any order,
we must sort them to preserve reproducible builds.
* gc.largeStackFramesMu. We don't discover until after SSA compilation
that a function's stack frame is gigantic.
Recording that error happens basically never,
but it does happen concurrently.
Fix with a low priority mutex and sorting.
* obj.Link.hashmu. ctxt.hash stores the mapping from
types.Syms (compiler symbols) to obj.LSyms (linker symbols).
It is accessed fairly heavily through all the phases.
This is the only heavily contended mutex.
* gc.signatlistmu. The global signatlist map is
populated with types through several of the concurrent phases,
including notably via ngotype during DWARF generation.
It is low priority for removal.
* gc.typepkgmu. Looking up symbols in the types package
happens a fair amount during backend compilation
and DWARF generation, particularly via ngotype.
This mutex helps us to avoid a broader mutex on types.Pkg.Syms.
It has low-to-moderate contention.
* types.internedStringsmu. gc AST to SSA conversion and
some SSA work introduce new autotmps.
Those autotmps have their names interned to reduce allocations.
That interning requires protecting types.internedStrings.
The autotmp names are heavily re-used, and the mutex
overhead and contention here are low, so it is probably
a worthwhile performance optimization to keep this mutex.
TESTING
I have been testing this code locally by running
'go install -race cmd/compile'
and then doing
'go build -a -gcflags=-c=128 std cmd'
for all architectures and a variety of compiler flags.
This obviously needs to be made part of the builders,
but it is too expensive to make part of all.bash.
I have filed #19962 for this.
REPRODUCIBLE BUILDS
This version of the compiler generates reproducible builds.
Testing reproducible builds also needs automation, however,
and is also too expensive for all.bash.
This is #19961.
Also of note is that some of the compiler flags used by 'toolstash -cmp'
are currently incompatible with concurrent backend compilation.
They still work fine with c=1.
Time will tell whether this is a problem.
NEXT STEPS
* Continue to find and fix races and bugs,
using a combination of code inspection, fuzzing,
and hopefully some community experimentation.
I do not know of any outstanding races,
but there probably are some.
* Improve testing.
* Improve performance, for many values of c.
* Integrate with cmd/go and fine tune.
* Support concurrent compilation with the -race flag.
It is a sad irony that it does not yet work.
* Minor code cleanup that has been deferred during
the last month due to uncertainty about the
ultimate shape of this CL.
PERFORMANCE
Here's the buried lede, at last. :)
All benchmarks are from my 8 core 2.9 GHz Intel Core i7 darwin/amd64 laptop.
First, going from tip to this CL with c=1 has almost no impact.
Comparing this CL to itself, from c=1 to c=2
improves real times 20-30%, costs 5-10% more CPU time,
and adds about 2% alloc.
The allocation increase comes from allocating more ssa.Caches.
Going beyond c=4 on my machine tends to increase CPU time and allocs
without impacting real time.
The CPU time numbers matter, because when there are many concurrent
compilation processes, that will impact the overall throughput.
The numbers above are in many ways the best case scenario;
we can take full advantage of all cores.
Fortunately, the most common compilation scenario is incremental
re-compilation of a single package during a build/test cycle.
Updates #15756
Change-Id: I6725558ca2069edec0ac5b0d1683105a9fff6bea
Reviewed-on: https://go-review.googlesource.com/40693 Reviewed-by: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Robert Griesemer <gri@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Alex Brainman [Wed, 26 Apr 2017 04:49:58 +0000 (14:49 +1000)]
os: do not report ModeDir for symlinks on windows
When using Lstat against symlinks that point to a directory,
the function returns FileInfo with both ModeDir and ModeSymlink set.
Change that to never set ModeDir if ModeSymlink is set.
Fixes #10424
Fixes #17540
Fixes #17541
Change-Id: Iba280888aad108360b8c1f18180a24493fe7ad2b
Reviewed-on: https://go-review.googlesource.com/41830 Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>