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

bin/traceur.js has insignificant diffs that confuse commits #423

Closed
johnjbarton opened this issue Nov 13, 2013 · 10 comments
Closed

bin/traceur.js has insignificant diffs that confuse commits #423

johnjbarton opened this issue Nov 13, 2013 · 10 comments
Assignees

Comments

@johnjbarton
Copy link
Contributor

I believe the diffs we see are caused by using src/node/inline-module.js inlineAndCompile() on the file list $(SRC) in
./traceur --out bin/traceur.js $(TFLAGS) $(SRC)

inlineAndCompile loads files async, then compiles them. The compiler uses generateUniqueIdentifier from src/codegeneration/UniqueIdentifierGenerator.js which issues numbers serially from zero. Compilation order changes cause identifier changes.

I'm not 100% because the diffs ought to be larger in most cases. But notice that the diffs do seem to be equal number of adds and deletes.

@johnjbarton
Copy link
Contributor Author

No, it looks like the list of files is loaded async then compiled as a unit in order.

@rosshadden
Copy link
Contributor

Should bin/traceur.js actually be in the repo? Typically generated files are offered outside of the repo, like on a gh-pages branch, or can be built from the source in the repo.

@arv
Copy link
Collaborator

arv commented Nov 13, 2013

@rosshadden You need a binary that is able to compile traceur itself. Since Traceur uses ES6 we need to keep this binary up to date (at least up to date enough to compile traceur).

@johnjbarton
Copy link
Contributor Author

@arv Do you know what causes the different event turns in traceur? If I print the oldest stack frame when generateUniqueIdentifer() is called I see changes, but I don't see what queues that first call. Its usually transformAny or transform.

@arv
Copy link
Collaborator

arv commented Nov 13, 2013

@johnjbarton Yeah, I would think most of these comes from the transformers (most commonly TempVarTransformer). These gets called from from the CodeLoader which is async. Maybe you could log the order in which the files get transformed? If that is not stable there is no way that the ids will be stable.

@johnjbarton
Copy link
Contributor Author

Here is a trace of the file loads and id generation.

A lot of the ids never appear in the generated source. So some of the diffs we don't like could be real: compiler changes could cause more or less extraneous id generation. The diffs that are surely unreal are those that appear after we clone master and build fresh: we should never see diffs if the compile is deterministic.

Changes to load order should result in id diffs and in blocks of generated code shifted. We mostly see the first sometimes the second.

Just now I cloned git@github.com:google/traceur-compiler.git on my linux box and on my Mac Air. Both gave diffs after
make clean force
make force
but the diffs differ ;-). The Mac Air diff clearly has options.js moved down in the file. Let me see if I can get a trace that is also different.

@johnjbarton
Copy link
Contributor Author

Here is the diff of the mac air and linux logs. Transform order differs.

2c2
< --------------------- eval contents of /work/FakeMaker/extension/third_party/traceur-compiler/bin/traceur.js
---
> --------------------- eval contents of /work/traceur-compiler/bin/traceur.js
19a20
> NodeLoader load ../src/semantics/ModuleAnalyzer.js
22d22
< NodeLoader load ../src/semantics/ModuleAnalyzer.js
49d48
< codeUnit.abort loadTextFile ../src/WebPageProject.js
54c53
< codeUnit.abort loadTextFile ../src/util/SourcePosition.js
---
> codeUnit.abort loadTextFile ../src/WebPageProject.js
62a62,63
> codeUnit.abort loadTextFile ../src/util/ErrorReporter.js
> codeUnit.abort loadTextFile ../src/util/SourcePosition.js
64d64
< codeUnit.abort loadTextFile ../src/syntax/IdentifierToken.js
66c66
< codeUnit.abort loadTextFile ../src/util/ErrorReporter.js
---
> codeUnit.abort loadTextFile ../src/syntax/IdentifierToken.js
75,78d74
< codeUnit.abort loadTextFile ../src/syntax/SourceFile.js
< NodeLoader load ../src/syntax/LineNumberTable.js
< NodeLoader load ../src/util/uid.js
< codeUnit.abort loadTextFile ../src/syntax/Token.js
82a79,82
> codeUnit.abort loadTextFile ../src/syntax/SourceFile.js
> NodeLoader load ../src/syntax/LineNumberTable.js
> NodeLoader load ../src/util/uid.js
> codeUnit.abort loadTextFile ../src/syntax/Token.js
90d89
< codeUnit.abort loadTextFile ../src/outputgeneration/TreeWriter.js
91a91,92
> codeUnit.abort loadTextFile ../src/outputgeneration/TreeWriter.js
> codeUnit.abort loadTextFile ../src/codegeneration/Compiler.js
95d95
< codeUnit.abort loadTextFile ../src/codegeneration/Compiler.js
119d118
< codeUnit.abort loadTextFile ../src/codegeneration/module/ModuleRequireVisitor.js
120a120
> codeUnit.abort loadTextFile ../src/codegeneration/module/ModuleRequireVisitor.js
137d136
< codeUnit.abort loadTextFile ../src/syntax/trees/ParseTreeType.js
140c139
< codeUnit.abort loadTextFile ../src/util/SourceRange.js
---
> codeUnit.abort loadTextFile ../src/syntax/trees/ParseTreeType.js
141a141
> codeUnit.abort loadTextFile ../src/util/SourceRange.js
143,144d142
< codeUnit.abort loadTextFile ../src/syntax/LineNumberTable.js
< codeUnit.abort loadTextFile ../src/util/uid.js
148c146,147
< codeUnit.abort loadTextFile ../src/syntax/ParseTreeVisitor.js
---
> codeUnit.abort loadTextFile ../src/syntax/LineNumberTable.js
> codeUnit.abort loadTextFile ../src/util/uid.js
150a150
> codeUnit.abort loadTextFile ../src/syntax/ParseTreeVisitor.js
153,154d152
< codeUnit.abort loadTextFile ../src/codegeneration/ArrowFunctionTransformer.js
< NodeLoader load ../src/codegeneration/FindInFunctionScope.js
156a155,156
> codeUnit.abort loadTextFile ../src/codegeneration/ArrowFunctionTransformer.js
> NodeLoader load ../src/codegeneration/FindInFunctionScope.js
179d178
< codeUnit.abort loadTextFile ../src/codegeneration/TemplateLiteralTransformer.js
181a181
> codeUnit.abort loadTextFile ../src/codegeneration/TemplateLiteralTransformer.js
191a192
> codeUnit.abort loadTextFile ../src/codegeneration/ComprehensionTransformer.js
199d199
< codeUnit.abort loadTextFile ../src/codegeneration/ComprehensionTransformer.js
201,202c201
< codeUnit.abort loadTextFile ../src/codegeneration/generator/ForInTransformPass.js
< codeUnit.abort loadTextFile ../src/codegeneration/generator/GeneratorTransformer.js
---
> codeUnit.abort loadTextFile ../src/codegeneration/generator/AsyncTransformer.js
204a204
> NodeLoader load ../src/codegeneration/generator/FallThroughState.js
205a206,207
> codeUnit.abort loadTextFile ../src/codegeneration/generator/ForInTransformPass.js
> codeUnit.abort loadTextFile ../src/codegeneration/generator/GeneratorTransformer.js
208,209d209
< codeUnit.abort loadTextFile ../src/codegeneration/generator/AsyncTransformer.js
< NodeLoader load ../src/codegeneration/generator/FallThroughState.js
212,217d211
< codeUnit.abort loadTextFile ../src/codegeneration/generator/ReturnState.js
< NodeLoader load ../src/codegeneration/generator/State.js
< codeUnit.abort loadTextFile ../src/syntax/trees/StateMachine.js
< NodeLoader load ../src/codegeneration/generator/TryState.js
< codeUnit.abort loadTextFile ../src/codegeneration/generator/FallThroughState.js
< codeUnit.abort loadTextFile ../src/codegeneration/generator/EndState.js
223a218
> NodeLoader load ../src/codegeneration/generator/State.js
225a221,224
> NodeLoader load ../src/codegeneration/generator/TryState.js
> codeUnit.abort loadTextFile ../src/codegeneration/generator/EndState.js
> codeUnit.abort loadTextFile ../src/codegeneration/generator/FallThroughState.js
> codeUnit.abort loadTextFile ../src/syntax/trees/StateMachine.js
227,228c226
< codeUnit.abort loadTextFile ../src/codegeneration/generator/State.js
< codeUnit.abort loadTextFile ../src/codegeneration/generator/TryState.js
---
> codeUnit.abort loadTextFile ../src/codegeneration/generator/ReturnState.js
231a230
> codeUnit.abort loadTextFile ../src/codegeneration/generator/CatchState.js
233d231
< codeUnit.abort loadTextFile ../src/codegeneration/generator/FinallyFallThroughState.js
234a233,234
> codeUnit.abort loadTextFile ../src/codegeneration/generator/FinallyFallThroughState.js
> codeUnit.abort loadTextFile ../src/codegeneration/generator/State.js
236d235
< codeUnit.abort loadTextFile ../src/codegeneration/generator/CatchState.js
237a237
> codeUnit.abort loadTextFile ../src/codegeneration/generator/TryState.js
243,244c243
< transformCodeUnit ../src/options.js
< transformCodeUnit ../src/WebPageProject.js
---
> transformCodeUnit ../src/semantics/ModuleAnalyzer.js
247c246,247
< transformCodeUnit ../src/semantics/ModuleAnalyzer.js
---
> transformCodeUnit ../src/options.js
> transformCodeUnit ../src/WebPageProject.js
366c366
< transformCodeUnit ../src/syntax/LineNumberTable.js
---
> transformCodeUnit ../src/syntax/KeywordToken.js
369,372d368
< transformCodeUnit ../src/util/uid.js
< transformCodeUnit ../src/syntax/KeywordToken.js
< generateUniqueIdentifier 83
< generateUniqueIdentifier 84
374a371,374
> transformCodeUnit ../src/syntax/LineNumberTable.js
> generateUniqueIdentifier 83
> generateUniqueIdentifier 84
> transformCodeUnit ../src/util/uid.js
483c483
< transformCodeUnit ../src/codegeneration/FindInFunctionScope.js
---
> transformCodeUnit ../src/codegeneration/ComprehensionTransformer.js
486c486
< transformCodeUnit ../src/codegeneration/ComprehensionTransformer.js
---
> transformCodeUnit ../src/codegeneration/FindInFunctionScope.js
539c539
< transformCodeUnit ../src/syntax/trees/StateMachine.js
---
> transformCodeUnit ../src/codegeneration/generator/FallThroughState.js
542c542
< transformCodeUnit ../src/codegeneration/generator/YieldState.js
---
> transformCodeUnit ../src/syntax/trees/StateMachine.js
545c545
< transformCodeUnit ../src/codegeneration/generator/ReturnState.js
---
> transformCodeUnit ../src/codegeneration/generator/YieldState.js
548c548
< transformCodeUnit ../src/codegeneration/generator/FallThroughState.js
---
> transformCodeUnit ../src/codegeneration/generator/ReturnState.js
551c551
< transformCodeUnit ../src/codegeneration/generator/State.js
---
> transformCodeUnit ../src/codegeneration/generator/BreakContinueTransformer.js
554c554
< transformCodeUnit ../src/codegeneration/generator/TryState.js
---
> transformCodeUnit ../src/codegeneration/generator/CatchState.js
557c557
< transformCodeUnit ../src/codegeneration/generator/BreakContinueTransformer.js
---
> transformCodeUnit ../src/codegeneration/generator/ConditionalState.js
560c560
< transformCodeUnit ../src/codegeneration/generator/CatchState.js
---
> transformCodeUnit ../src/codegeneration/generator/FinallyFallThroughState.js
563c563
< transformCodeUnit ../src/codegeneration/generator/ConditionalState.js
---
> transformCodeUnit ../src/codegeneration/generator/FinallyState.js
566c566
< transformCodeUnit ../src/codegeneration/generator/FinallyFallThroughState.js
---
> transformCodeUnit ../src/codegeneration/generator/State.js
569c569
< transformCodeUnit ../src/codegeneration/generator/FinallyState.js
---
> transformCodeUnit ../src/codegeneration/generator/StateAllocator.js
572c572
< transformCodeUnit ../src/codegeneration/generator/StateAllocator.js
---
> transformCodeUnit ../src/codegeneration/generator/SwitchState.js
575c575
< transformCodeUnit ../src/codegeneration/generator/SwitchState.js
---
> transformCodeUnit ../src/codegeneration/generator/TryState.js

@johnjbarton
Copy link
Contributor Author

It looks like we transform the files on the command line in order then src/traceur.js. The remainder are not in order. But the remainder are all transformed after all of the loads. So there should be no penalty for transforming in some fixed order.

@johnjbarton
Copy link
Contributor Author

@arv From what I can tell the dependency graph in module-loader is implicit in the order of entries to the arraymap "cache". The first codeunit arrives from the network (fs) and is pushed into the cache. It is parsed and its dependencies are pushed and queued for loading. When all of the values in the cache are parsed, we begin at the front and transform. Thus we are assured that all of the dependences are available during transform and immediate dependencies are ordered but the order of dependencies of dependencies vary with load order.

I think that maintaining the dependency graph would solve the current issue and it would be useful for diagnostic messages. Each codeunit would have an array of dependencies ordered by the parse and the transform would operate in that order (depth first). I don't think it would add significant overhead, just an array of pointers per codeunit. Am I on the right track?

@arv
Copy link
Collaborator

arv commented Nov 14, 2013

That sounds like it would work.
On Nov 13, 2013 9:06 PM, "johnjbarton" notifications@github.com wrote:

@arv https://github.com/arv From what I can tell the dependency graph
in module-loader is implicit in the order of entries to the arraymap
"cache". The first codeunit arrives from the network (fs) and is pushed
into the cache. It is parsed and its dependencies are pushed and queued for
loading. When all of the values in the cache are parsed, we begin at the
front and transform. Thus we are assured that all of the dependences are
available during transform and immediate dependencies are ordered but the
order of dependencies of dependencies vary with load order.

I think that maintaining the dependency graph would solve the current
issue and it would be useful for diagnostic messages. Each codeunit would
have an array of dependencies ordered by the parse and the transform would
operate in that order (depth first). I don't think it would add significant
overhead, just an array of pointers per codeunit. Am I on the right track?


Reply to this email directly or view it on GitHubhttps://github.com//issues/423#issuecomment-28453794
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants