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

Jump to bottom

cmd/compile: non-deterministic build blocking use of toolstash-check #19872

Closed
mdempsky opened this issue Apr 7, 2017 · 3 comments
Closed

cmd/compile: non-deterministic build blocking use of toolstash-check #19872

mdempsky opened this issue Apr 7, 2017 · 3 comments
Labels
FrozenDueToAge

Comments

@mdempsky
Copy link
Member

mdempsky commented Apr 7, 2017

At master, running toolstash-check on a CL that just adds a comment to cmd/compile causes this failure:

# html/template
toolstash: compiler output differs, even with optimizers disabled (-N)

inconsistent log line:
/tmp/go-build151744301/html/template.a.log:84904:
        type..namedata.**template.Template. t=8 dupok size=22
/tmp/go-build151744301/html/template.a.stash.log:84904:
        type..namedata.*[]uint8. t=8 dupok size=11

@griesemer is seeing the same thing. It appears to be from CL 39710.

@josharian It looks like LongString isn't a total ordering either. :(
@gopherbot
Copy link

CL https://golang.org/cl/39871 mentions this issue.

@josharian
Copy link
Contributor

I'm an idiot. I missed the obviously correct ordering, which is to do what the actual export code (dtypesym) does.

@gopherbot
Copy link

CL https://golang.org/cl/39915 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 7, 2017
@josharian
cmd/compile: make typenamesym do less work
6d9c77f 
This is a re-roll of CL 39710,
which broke deterministic builds.

typenamesym is called from three places:
typename, ngotype, and Type.Symbol.
Only in typename do we actually need a Node.
ngotype and Type.Symbol require only a Sym.
And writing the newly created Node to
Sym.Def is unsafe in a concurrent backend.
Rather than use a mutex protect to Sym.Def,
make typenamesym not touch Sym.Def.

The assignment to Sym.Def was serving a second purpose,
namely to prevent duplicate entries on signatlist.
Preserve that functionality by switching signatlist to a map.
This in turn requires that we sort signatlist
when exporting it, to preserve reproducibility.

We sort using exactly the same mechanism
that the export code (dtypesym) uses.
Failure to do that led to non-deterministic builds (#19872).
Since we've already calculated the Type's export name,
we could pass it to dtypesym, sparing it a bit of work.
That can be done as a future optimization.

Updates #15756

name       old alloc/op      new alloc/op      delta
Template        39.2MB ± 0%       39.3MB ± 0%    ~     (p=0.075 n=10+10)
Unicode         29.8MB ± 0%       29.8MB ± 0%    ~     (p=0.393 n=10+10)
GoTypes          113MB ± 0%        113MB ± 0%  +0.06%  (p=0.027 n=10+8)
SSA             1.25GB ± 0%       1.25GB ± 0%  +0.05%  (p=0.000 n=8+10)
Flate           25.3MB ± 0%       25.3MB ± 0%    ~     (p=0.105 n=10+10)
GoParser        31.7MB ± 0%       31.8MB ± 0%    ~     (p=0.165 n=10+10)
Reflect         78.2MB ± 0%       78.2MB ± 0%    ~     (p=0.190 n=10+10)
Tar             26.6MB ± 0%       26.6MB ± 0%    ~     (p=0.481 n=10+10)
XML             42.2MB ± 0%       42.2MB ± 0%    ~     (p=0.968 n=10+9)

name       old allocs/op     new allocs/op     delta
Template          384k ± 1%         386k ± 1%  +0.43%  (p=0.019 n=10+10)
Unicode           320k ± 0%         321k ± 0%  +0.36%  (p=0.015 n=10+10)
GoTypes          1.14M ± 0%        1.14M ± 0%  +0.33%  (p=0.000 n=10+8)
SSA              9.69M ± 0%        9.71M ± 0%  +0.18%  (p=0.000 n=10+9)
Flate             233k ± 1%         233k ± 1%    ~     (p=0.481 n=10+10)
GoParser          315k ± 1%         316k ± 1%    ~     (p=0.113 n=9+10)
Reflect           979k ± 0%         979k ± 0%    ~     (p=0.971 n=10+10)
Tar               250k ± 1%         250k ± 1%    ~     (p=0.481 n=10+10)
XML               391k ± 1%         392k ± 0%    ~     (p=1.000 n=10+9)

Change-Id: Ia9f21cc29c047021fa8a18c2a3d861a5146aefac
Reviewed-on: https://go-review.googlesource.com/39915
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@josharian josharian mentioned this issue Apr 13, 2017
Open
lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
@mdempsky @lparth
Revert "cmd/compile: make typenamesym do less work"
e796aab 
This reverts commit 91433eb.

Reason for revert: broke deterministic build.

Fixes golang#19872.

Change-Id: Ia1a0fc651b818bdf69454df43bd189689c0348a0
Reviewed-on: https://go-review.googlesource.com/39871
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
@josharian @lparth
cmd/compile: make typenamesym do less work
5a64ba4 
This is a re-roll of CL 39710,
which broke deterministic builds.

typenamesym is called from three places:
typename, ngotype, and Type.Symbol.
Only in typename do we actually need a Node.
ngotype and Type.Symbol require only a Sym.
And writing the newly created Node to
Sym.Def is unsafe in a concurrent backend.
Rather than use a mutex protect to Sym.Def,
make typenamesym not touch Sym.Def.

The assignment to Sym.Def was serving a second purpose,
namely to prevent duplicate entries on signatlist.
Preserve that functionality by switching signatlist to a map.
This in turn requires that we sort signatlist
when exporting it, to preserve reproducibility.

We sort using exactly the same mechanism
that the export code (dtypesym) uses.
Failure to do that led to non-deterministic builds (golang#19872).
Since we've already calculated the Type's export name,
we could pass it to dtypesym, sparing it a bit of work.
That can be done as a future optimization.

Updates golang#15756

name       old alloc/op      new alloc/op      delta
Template        39.2MB ± 0%       39.3MB ± 0%    ~     (p=0.075 n=10+10)
Unicode         29.8MB ± 0%       29.8MB ± 0%    ~     (p=0.393 n=10+10)
GoTypes          113MB ± 0%        113MB ± 0%  +0.06%  (p=0.027 n=10+8)
SSA             1.25GB ± 0%       1.25GB ± 0%  +0.05%  (p=0.000 n=8+10)
Flate           25.3MB ± 0%       25.3MB ± 0%    ~     (p=0.105 n=10+10)
GoParser        31.7MB ± 0%       31.8MB ± 0%    ~     (p=0.165 n=10+10)
Reflect         78.2MB ± 0%       78.2MB ± 0%    ~     (p=0.190 n=10+10)
Tar             26.6MB ± 0%       26.6MB ± 0%    ~     (p=0.481 n=10+10)
XML             42.2MB ± 0%       42.2MB ± 0%    ~     (p=0.968 n=10+9)

name       old allocs/op     new allocs/op     delta
Template          384k ± 1%         386k ± 1%  +0.43%  (p=0.019 n=10+10)
Unicode           320k ± 0%         321k ± 0%  +0.36%  (p=0.015 n=10+10)
GoTypes          1.14M ± 0%        1.14M ± 0%  +0.33%  (p=0.000 n=10+8)
SSA              9.69M ± 0%        9.71M ± 0%  +0.18%  (p=0.000 n=10+9)
Flate             233k ± 1%         233k ± 1%    ~     (p=0.481 n=10+10)
GoParser          315k ± 1%         316k ± 1%    ~     (p=0.113 n=9+10)
Reflect           979k ± 0%         979k ± 0%    ~     (p=0.971 n=10+10)
Tar               250k ± 1%         250k ± 1%    ~     (p=0.481 n=10+10)
XML               391k ± 1%         392k ± 0%    ~     (p=1.000 n=10+9)

Change-Id: Ia9f21cc29c047021fa8a18c2a3d861a5146aefac
Reviewed-on: https://go-review.googlesource.com/39915
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@golang golang locked and limited conversation to collaborators Apr 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mdempsky @josharian @gopherbot