Refactor logic and optimizations in rebar_erlc_compiler:doterl_compile/4 #467

ghost · 2015-03-29T08:24:39Z

Change format of erlcinfo so that it is more robust and contains enough info for deciding which files need to be recompiled.
Add tests.
Remove unneeded code.

ghost · 2015-03-29T11:38:09Z

Thanks for the patch.

@norton and @nevar, can you check that the changes don't break your projects?

You forgot to append your name in THANKS.

Please do not rewrite the branch for any followup fixes, because it's a large change and, given GitHub's review system, it's the only way to review further changes. That means, make any fixes in extra commits, even though it's many commits.

Can you separately explain the include_path and store_erlc_info changes in more detail, in order to leave no room for interpretation? If it makes sense, maybe as a code comment.

ghost · 2015-03-29T21:36:50Z

Please do not rewrite the branch for any followup fixes, because it's a large change and, given GitHub's review system, it's the only way to review further changes. That means, make any fixes in extra commits, even though it's many commits.

Don't worry, I am used to doing review by sending just fixups. I have made a force push shortly after initial pull request only because nobody had probably looked at it at that time. Although here the fixups will bring some merge conflicts during final rebase, but I should manage.

Actually I am also used to commenting on particular lines of source code instead of writing comments to conversation sections. So if you don't mind I will continue the review in particular commits, where appropriate.

ghost · 2015-03-29T23:01:18Z

I think a have adressed all your comments. Either by sending a fixup or by commenting to corresponding commit.

norton · 2015-03-30T02:31:34Z

@Tuncer Here is the result with one project - ubf. I did not check the details of the failure.

$ erl --version
Erlang/OTP 17 [erts-6.3] [source] [64-bit] [smp:4:4] [async-threads:10] [hipe] [kernel-poll:false]

Eshell V6.3  (abort with ^G)
1> 

$ git clone git@github.com:ubf/ubf.git

$ cd ubf

$ cp <<path-to-rebar>> ./rebar

$ ./rebar --version
rebar 2.5.1 17 20150330_022056 git 2.5.1-165-g82a1756

$ ./rebar get

$ make 
./rebar compile
==> qc (compile)
ERROR: compile failed while processing /Users/norton/tmp/ubf/deps/qc: {'EXIT',
    {function_clause,
        [{filename,join,
             [{error,bad_name},"triq_statem.hrl"],
             [{file,"filename.erl"},{line,406}]},
         {rebar_erlc_compiler,process_attr,3,
             [{file,"src/rebar_erlc_compiler.erl"},{line,591}]},
         {rebar_erlc_compiler,parse_attrs,2,
             [{file,"src/rebar_erlc_compiler.erl"},{line,559}]},
         {rebar_erlc_compiler,modify_erlcinfo,4,
             [{file,"src/rebar_erlc_compiler.erl"},{line,389}]},
         {rebar_erlc_compiler,'-modify_erlcinfo/4-fun-0-',4,
             [{file,"src/rebar_erlc_compiler.erl"},{line,396}]},
         {lists,foreach,2,[{file,"lists.erl"},{line,1336}]},
         {rebar_erlc_compiler,modify_erlcinfo,4,
             [{file,"src/rebar_erlc_compiler.erl"},{line,394}]},
         {rebar_erlc_compiler,'-modify_erlcinfo/4-fun-0-',4,
             [{file,"src/rebar_erlc_compiler.erl"},{line,396}]}]}}
make: *** [compile] Error 1

ghost · 2015-03-30T11:35:01Z

Don't worry, I am used to doing review by sending just fixups. I have made a force push shortly after initial pull request only because nobody had probably looked at it at that time. Although here the fixups will bring some merge conflicts during final rebase, but I should manage.

I don't mind having the fixups as extra commits, though you'd have to fix the commit message in that case.

Actually I am also used to commenting on particular lines of source code instead of writing comments to conversation sections. So if you don't mind I will continue the review in particular commits, where appropriate.

Of course, it's just that Github's review system is insufficient, and I need a way to keep track of review progress. There's a lot of commits, but I'll try to not miss any of the inline comments.

nevar · 2015-03-30T12:33:03Z

src/rebar_erlc_compiler.erl

+needed_files(G, OutDir, [Source|Rest]) ->
+    Target = target_base(OutDir, Source) ++ ".beam",
+    Needed = digraph:vertex(G, Source) > {Source, filelib:last_modified(Target)},
+    [Source||Needed] ++ needed_files(G, OutDir, Rest);


Why not:

needed_files(G, OutDir, ErlFileList) -> [File || File <- ErlFileList, digraph:vertex(G, File) > {File, beam_last_modified(OutDir, File)}]. beam_last_modified(OutDir, Source) -> filelib:last_modified(target_base(OutDir, Source) ++ ".beam").

I have solved it very similarly with lists:filter as per suggestion by @ferd.

nevar · 2015-03-30T12:47:25Z

Great work. Looked code. Not much only part related to erl_first_files.

ghost · 2015-04-01T16:13:10Z

So I think that I have adressed all issues, except that andalso thing in 1243359, which I will repair in fixup during rebase. So if anyone has any comments to particular commits (or fixups), I will wait cca 24 hours and then I will squash all fixups together, therefore all comments to current commits will be lost. Then I want to recheck the issue reported by @norton, although I don't have a clue how to debug it yet. If ubf is a private repository, can you give some hints what possible unusual you might be doing there?

norton · 2015-04-01T17:19:26Z

@kejv Oops ... I used a private url for the cloning recipe. ubf is not a private repository and is available at GitHub. I updated the recipe above.

ghost · 2015-04-02T11:42:38Z

Let's get the regression(s) fixed and move this forward. We can address subjective code issues independently.

Its case statement is noop.

Also pas only InclDirs into init_erlcinfo as this is the only thing from Config/ErlOpts needed there.

Otherwise the code cannot be safely refactored, since potential new erros slip unnoticed.

As erlcinfo is only an internal optimization mechanism, user should be only minimally bothered by issues with it. Especially it shouldn't ever cause a fatal error and if it cannot be restored properly, only a warning (not error) message should be emitted (if any). Also it probably doesn't make sense for this warning message to be too detailed - just state that something went wrong and silently delete the erlcinfo file.

Current separation of part of the logic into include_path obscures the purpose of Dirs (see expand_file_names). Moreover for each particular Erl its source file was included twice in Dirs, which is now corrected. Also InclDirs (specified in erl_opts) as part of erlcinfo since they can affect resulting graph, which needs to be therefore regenerated when InclDirs change. See added test as an example.

ghost · 2015-04-04T23:02:59Z

I have squashed all the fixups, fixed merge conflicts along the way and added one additional fixup for that andalso thing as agreed (look also how it affects code in ef0165f). Hopefully everything should be ok now.

ghost · 2015-04-04T23:11:07Z

@norton I have looked at the problem you reported and it really is caused by d61b51b. At that commit I have stopped silently supressing some errors from process_attr/3 and this one of yours suddenly popped up (it is present in current master). I will send a fix with explanation.

ghost · 2015-04-06T09:47:46Z

@kejv wrote:

@norton I have looked at the problem you reported and it really is caused by d61b51b. At that commit I have stopped silently supressing some errors from process_attr/3 and this one of yours suddenly popped up (it is present in current master). I will send a fix with explanation.

Yeah, I found+fixed the initial problems with include_lib only after disabling the catch-all, and then commented it shouldn't unconditionally suppress all errors. I like that we don't do that anymore, and I wonder if we have a means of reporting any other error in a nicer way. I'm thinking of anything that might throw in AttrName = erl_syntax:atom_value(erl_syntax:attribute_name(Form)). We probably cannot, but it doesn't hurt to consider our options.

Other than that, this is ready to merge, isn't it?

ghost · 2015-04-06T10:56:34Z

I wonder if we have a means of reporting any other error in a nicer way. I'm thinking of anything that might throw in AttrName = erl_syntax:atom_value(erl_syntax:attribute_name(Form))

As I said earlier I have experimented a little with syntax errors in compile attributes, hoping that I will get exception here in process_attr/2 instead of in actual compilation, but I didn't (at least the error messages were the same as from erlc directly). So if there's a source of rebar exceptions here I don't of it.

ghost · 2015-04-06T18:32:06Z

It would be easier to match ok | {error, Reason}, but the way erl_syntax's api works (see erlang-questions thread), we can not know what exception it may throw. There may be other code out there that will trigger an exception here, and if we can present such an error in a user friendlier way, we should do it. That said, I don't consider this a merge blocker, so it's fine to merge as is.

ghost · 2015-04-06T20:09:52Z

Ok then. I will do one final rebase and then it can be merged.

When the source file changes it could happen that some of its dependecies get removed. In that case we should remove these former dependencies from the graph, so that they don't influence recompilation of the source file anymore.

- use filelib:last_modified/1 instead of date/0 and time/0 combo in source file vertices - simplify store_erlcinfo/2 - document init_erlcinfo/2

Optimizing for best compression could be very slow for large projects, so optimize rather for the speed. erlcinfo file isn't that huge anyway and moreover difference between levels 2 and 9 isn't that big in practice (although there's quite big difference between level 2 and no compression at all).

The precise algorithm is now following: - Decide which files need to be compiled first (ErlFirstFiles). - Split AllErlFiles which are not in ErlFirstFiles into two groups, one consisting of files some other file depends on (DepErls) and the rest (OtherErls). - Final FirstErls are obtained as ErlFirstFiles (in original order) plus toposorted DepErls. OtherErls are left as is since they can be compiled in any (random) order. Also create the erlcinfo graph as acyclic, since otherwise the toposort could not work and we don't want to have cycles among dependencies anyway.

These times already contain max modified info of their dependencies. Therefore we no longer have to check in internal_erl_compile whether the file really needs recompiling, which simplifies the flow somewhat, because the work with dependency graph is now localized to much smaller space then before.

@Tuncer

As per @Tuncer: "That's most likely a vestige of package module support".

Since process_attr/3 searches source code for attributes, it can happen that it finds an attribute which is eventually not needed by the compilation, e.g. hidden by ifdef macro - see enclosed test. Such attribute can reference a file which is not in the path or which even doesn't exist at all, and current code doesn't expect such a situation. We fix things by simply ignoring such files.

Its format was (repeatedly) changed in previous commits.

norton · 2015-04-06T22:27:58Z

@kejv changes look good 👍 Thank you.

$ ./rebar --version
rebar 2.5.1 17 20150406_222000 git 2.5.1-174-ge17d30a

ghost · 2015-04-07T19:45:32Z

@ferd, okay to merge?

ferd · 2015-04-07T20:33:03Z

Let's see how it goes and if it breaks stuff!

Refactor logic and optimizations in rebar_erlc_compiler:doterl_compile/4

carlosedp · 2015-04-22T20:02:29Z

I saw the same problem reported on #479 where my project uses "include_lib" statements when I updated rebar.
This one works perfecly:
> ./rebar --version rebar 2.5.1 17 20150324_112243 git 2.5.1-141-g02de46d-dirty

This one gives me error:
> ./rebar --version rebar 2.5.1 17 20150422_145452 git 2.5.1-181-g56ac194-dirty

The error is:
==> dccaserver (compile) ERROR: compile failed while processing e:/cygwin17/home/cpaula/dcca-server-OTP/apps/dccaserver: {'EXIT', {function_clause, [{filename,join,[[]],[{file,"filename.erl"},{line,392}]}, {rebar_erlc_compiler,expand_include_lib_path,1, [{file,"src/rebar_erlc_compiler.erl"},{line,647}]}, {rebar_erlc_compiler,process_attr,3, [{file,"src/rebar_erlc_compiler.erl"},{line,597}]}, {rebar_erlc_compiler,parse_attrs,2, [{file,"src/rebar_erlc_compiler.erl"},{line,565}]}, {rebar_erlc_compiler,modify_erlcinfo,4, [{file,"src/rebar_erlc_compiler.erl"},{line,400}]}, {rebar_erlc_compiler,'-update_erlcinfo_fun/2-fun-0-',4, [{file,"src/rebar_erlc_compiler.erl"},{line,359}]}, {lists,foldl,3,[{file,"lists.erl"},{line,1261}]}, {rebar_erlc_compiler,init_erlcinfo,2, [{file,"src/rebar_erlc_compiler.erl"},{line,349}]}]}} Makefile:25: recipe for target 'compile' failed make: *** [compile] Error 1

ghost · 2015-04-25T20:36:58Z

@carlosedp see #485

nevar reviewed Mar 30, 2015
View reviewed changes

David Kubecka added 8 commits April 5, 2015 00:15

Allow running specific retest suites

eb79622

Remove useless compile/3

f61ca47

Its case statement is noop.

Basic test for erlc (re)compiling and erlcinfo

b0320ce

Stop passing useless Config into erlcinfo_file

fd482b6

Also pas only InclDirs into init_erlcinfo as this is the only thing from Config/ErlOpts needed there.

Stop silently supressing errors from process_attr/2

d61b51b

Otherwise the code cannot be safely refactored, since potential new erros slip unnoticed.

Extract common pattern from rebar_erlc_compiler:process_attr

ccdfc1a

David Kubecka added 6 commits April 6, 2015 22:10

Avoid extra list traversal when checking for modified source files

13fe77d

Additional minor refactor in erlcinfo

9e33daa

- use filelib:last_modified/1 instead of date/0 and time/0 combo in source file vertices - simplify store_erlcinfo/2 - document init_erlcinfo/2

David Kubecka added 4 commits April 6, 2015 22:10

Remove obsolete way of generating target ebin subdirectories

ee5a75c

As per @Tuncer: "That's most likely a vestige of package module support".

Raise version of erlcinfo

d62624c

Its format was (repeatedly) changed in previous commits.

append myself to THANKS

e17d30a

ferd added a commit that referenced this pull request Apr 7, 2015

Merge pull request #467 from kejv/support-parse-transforms2

b557e5f

Refactor logic and optimizations in rebar_erlc_compiler:doterl_compile/4

ferd merged commit b557e5f into rebar:master Apr 7, 2015

tsloughter mentioned this pull request Apr 7, 2015

merge rebar2's Refactor logic and optimizations in rebar_erlc_compiler:doterl_compile/4 #467 erlang/rebar3#323

Merged

ghost deleted the support-parse-transforms2 branch April 13, 2015 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor logic and optimizations in rebar_erlc_compiler:doterl_compile/4 #467

Refactor logic and optimizations in rebar_erlc_compiler:doterl_compile/4 #467

ghost commented Mar 29, 2015

ghost commented Mar 29, 2015

ghost commented Mar 29, 2015

ghost commented Mar 29, 2015

norton commented Mar 30, 2015

ghost commented Mar 30, 2015

nevar Mar 30, 2015

ghost Mar 31, 2015

nevar commented Mar 30, 2015

ghost commented Apr 1, 2015

norton commented Apr 1, 2015

ghost commented Apr 2, 2015

ghost commented Apr 4, 2015

ghost commented Apr 4, 2015

ghost commented Apr 6, 2015

ghost commented Apr 6, 2015

ghost commented Apr 6, 2015

ghost commented Apr 6, 2015

norton commented Apr 6, 2015

ghost commented Apr 7, 2015

ferd commented Apr 7, 2015

carlosedp commented Apr 22, 2015

ghost commented Apr 25, 2015

Refactor logic and optimizations in rebar_erlc_compiler:doterl_compile/4 #467

Refactor logic and optimizations in rebar_erlc_compiler:doterl_compile/4 #467

Conversation

ghost commented Mar 29, 2015

ghost commented Mar 29, 2015

ghost commented Mar 29, 2015

ghost commented Mar 29, 2015

norton commented Mar 30, 2015

ghost commented Mar 30, 2015

nevar Mar 30, 2015

Choose a reason for hiding this comment

ghost Mar 31, 2015

Choose a reason for hiding this comment

nevar commented Mar 30, 2015

ghost commented Apr 1, 2015

norton commented Apr 1, 2015

ghost commented Apr 2, 2015

ghost commented Apr 4, 2015

ghost commented Apr 4, 2015

ghost commented Apr 6, 2015

ghost commented Apr 6, 2015

ghost commented Apr 6, 2015

ghost commented Apr 6, 2015

norton commented Apr 6, 2015

ghost commented Apr 7, 2015

ferd commented Apr 7, 2015

carlosedp commented Apr 22, 2015

ghost commented Apr 25, 2015