Ensure that sorting of variables is stable #681
Conversation
compiler/utils.go
Outdated
| if s[i].Name() != s[j].Name() { | ||
| return s[i].Name() < s[j].Name() | ||
| } | ||
| return s[i].Pos() < s[j].Pos() |
There was a problem hiding this comment.
More often than not, I think names will differ. Only when they don't, do you want to sort by pos. Therefore, I think it'd be more readable to make the less frequent case indented, by flipping them:
if s[i].Name() == s[j].Name() {
return s[i].Pos() < s[j].Pos()
}
return s[i].Name() < s[j].Name()This is equivalent. Just a style suggestion.
compiler/utils.go
Outdated
| @@ -650,5 +650,8 @@ func (s varsByName) Swap(i, j int) { | |||
| } | |||
|
|
|||
| func (s varsByName) Less(i, j int) bool { | |||
There was a problem hiding this comment.
varsByName is being used in exactly one place in this package. We're requiring Go 1.8, that means we have sort.Slice available to us. Let's use it.
It will bring this code closer to where it's being used, making it more readable in the relevant context.
If names are equal, sort ties by position. Fixes gopherjs#679
|
Thanks -- updated PR based on suggestions. |
|
LGTM. I tested it, it helps improve reproducibility of generated code. It hasn't resolved the issue fully for me, but I can see improvement. Compare before vs after. It seems the only remaining issue for me after this PR is the order of variables declared in Thanks a lot for the fix! |
|
I've tested some local changes, and I found that if I make the following additional changes to diff --git a/compiler/compiler.go b/compiler/compiler.go
index 460293d..d5e21e5 100644
--- a/compiler/compiler.go
+++ b/compiler/compiler.go
@@ -9,6 +9,7 @@ import (
"go/token"
"go/types"
"io"
+ "sort"
"strings"
"github.com/gopherjs/gopherjs/compiler/prelude"
@@ -190,8 +191,12 @@ func WritePkgCode(pkg *Archive, dceSelection map[*Decl]struct{}, minify bool, w
}
vars := []string{"$pkg = {}", "$init"}
var filteredDecls []*Decl
+ sort.Slice(pkg.Declarations, func(i, j int) bool {
+ return pkg.Declarations[i].FullName < pkg.Declarations[j].FullName
+ })
for _, d := range pkg.Declarations {
if _, ok := dceSelection[d]; ok {
+ sort.Strings(d.Vars)
vars = append(vars, d.Vars...)
filteredDecls = append(filteredDecls, d)
}Then the generated output is completely deterministic for me! Very exciting! (Not meant to be a part of this PR, that code still needs to be verified to generate correct code, tested, cleaned up and reviewed... I just wanted to share some relevant findings, that we're very close to full reproducibility as far as I can tell.) |
|
Awesome, that's great to hear! If the determinism is resolved and the vendor issue is resolved as well, then I think that will be sufficient to guarantee completely self-contained and consistent generated output for a given revision of a product, which is great! |
|
@neelance Do you have a chance to take a look at this? It's a small change, but I wanted to make sure you see it before we merge in case you have some thoughts. You've worked on related changes in the past, so it's FYI as well. |
|
Thanks for taking a look Richard, and thank you for the contribution @nmiyake! We're very close now to fully deterministic output! |
|
@shurcooL Any followup implementation of your suggestion above for further improving determinism? |
|
I tried to look into it briefly, and had the following changes locally for investigation: diff --git a/compiler/compiler.go b/compiler/compiler.go
index ad3af54..203bb96 100644
--- a/compiler/compiler.go
+++ b/compiler/compiler.go
@@ -13,6 +13,8 @@ import (
"github.com/gopherjs/gopherjs/compiler/prelude"
"golang.org/x/tools/go/gcimporter15"
+
+ "github.com/shurcooL/go-goon"
)
var sizes32 = &types.StdSizes{WordSize: 4, MaxAlign: 8}
@@ -190,12 +192,18 @@ func WritePkgCode(pkg *Archive, dceSelection map[*Decl]struct{}, minify bool, w
}
vars := []string{"$pkg = {}", "$init"}
var filteredDecls []*Decl
+ //sort.Slice(pkg.Declarations, func(i, j int) bool {
+ // return pkg.Declarations[i].FullName < pkg.Declarations[j].FullName
+ //})
for _, d := range pkg.Declarations {
if _, ok := dceSelection[d]; ok {
vars = append(vars, d.Vars...)
filteredDecls = append(filteredDecls, d)
}
}
+ goon.DumpExpr(vars)
+ //sort.Strings(vars)
+ //goon.DumpExpr(vars)
if _, err := w.Write(removeWhitespace([]byte(fmt.Sprintf("\tvar %s;\n", strings.Join(vars, ", "))), minify)); err != nil {
return err
}However, I didn't arrive at definitive results and knew it needed further investigation. I didn't have time, so I stashed the changes. I haven't had a change to revisit this issue since. |
If names are equal, sort ties by position.
Fixes #679