[#235] Single pass over forms #261
Conversation
| @@ -34,6 +34,7 @@ | |||
| %% upstream version: | |||
| %% https://github.com/massemanet/redbug/pull/10 | |||
| , [ {redbug, {git, "https://github.com/robertoaloi/redbug.git", {branch, "relax-beam-lib-error-matching"}}} | |||
| , {eflame, {git, "https://github.com/jfacorro/eflame.git", {branch, "various.improvements"}}} | |||
There was a problem hiding this comment.
I would prefer to bring in the upstream version, if possible.
What are the included changes?
Are you pull-requesting them to upstream?
There was a problem hiding this comment.
This is the PR with all the improvements.
src/els_parser.erl
Outdated
| {'ok', erl_syntax:forms() | none, integer()} | ||
| | {'eof', integer()} | ||
| | {'error', any(), integer()}. | ||
| parse_form(Dev, L0, Parser, _Options) -> |
There was a problem hiding this comment.
Can we please use the original naming for the variables? E.g. IoDevice instead of Dev, StartLoc instead of L0, Tokens instead of Ts, etc?
src/els_parser.erl
Outdated
| parse_form(Dev, L0, Parser, _Options) -> | ||
| case io:scan_erl_form(Dev, "", L0) of | ||
| {ok, Ts, L1} -> | ||
| case catch {ok, Parser(Ts, undefined)} of |
There was a problem hiding this comment.
Please don't use the deprecated catch. Use try/catch instead.
src/els_parser.erl
Outdated
| -spec find_attribute_pois(erl_parse:abstract_form(), [erl_scan:token()]) -> | ||
| [poi()]. | ||
| find_attribute_pois(Form, Tokens) -> | ||
| case erl_syntax:type(Form) of | ||
| attribute -> | ||
| case erl_syntax_lib:analyze_attribute(Form) of | ||
| case catch erl_syntax_lib:analyze_attribute(Form) of |
There was a problem hiding this comment.
Same here. Please don't use the old-style catch.
src/els_parser.erl
Outdated
| | none, integer()} | ||
| | {'eof', integer()} | ||
| | {'error', any(), integer()}. | ||
| parse_form(Dev, L0, Options) -> |
There was a problem hiding this comment.
This seems a re-implementation of els_dodger:parse_form/3. Why not to expose that one, instead?
There was a problem hiding this comment.
Sorry, I was misreading the code. Ignore this comment.
There was a problem hiding this comment.
An alternative option would have been to add a callback function to the Options list which could have been executed from within the els_dodger, and have the parse_form/4 return the final accumulator together with the list of forms.
There was a problem hiding this comment.
That would have required a change in the implementation of dodger which I was trying to avoid.
src/els_parser.erl
Outdated
| ), | ||
| {ok, Acc} | ||
| {error, _IoErr, _L1} = Err -> Err; | ||
| {error, _Reason} -> {eof, L0}; % This is probably encoding problem |
There was a problem hiding this comment.
I know the (missing an article) comment comes from the dodger, but I would get rid of it and, instead, add a comment to the top of this function explaining this is a simplified version of the respective dodger function.
src/els_parser.erl
Outdated
| fun(T, Acc) -> | ||
| [do_points_of_interest(T)|Acc] | ||
| end, [], Tree)). | ||
| FoldFun = fun(T, Acc) -> do_points_of_interest(T) ++ Acc end, |
There was a problem hiding this comment.
Do we need the ++ in here? Or can we live with nested lists that we can flatten at the end?
There was a problem hiding this comment.
Do you mean at the end as in parse/1 or at the end of the fold? If it's the former then I would agree, if it's the latter, I think it's much clearer what's happening if we use the ++.
src/els_parser.erl
Outdated
| , [EndLoc, ErrorInfo] | ||
| ), | ||
| {ok, Acc} | ||
| {error, _IoErr, _L1} = Err -> Err; |
There was a problem hiding this comment.
Considering we are pattern matching on {ok, ...} above, maybe we can avoid the defensive style altogether here.
There was a problem hiding this comment.
The result from this function is processed by the code in els_dodger which is pattern matching and handling each of these results though.
src/els_parser.erl
Outdated
| {ok, Ts, L1} -> | ||
| case catch {ok, Parser(Ts, undefined)} of | ||
| {ok, F} -> | ||
| POIs = [find_attribute_pois(F, Ts), points_of_interest(F)], |
There was a problem hiding this comment.
Now that we have the single pass, it probably makes more sense to check here whether we are dealing with an attribute and execute the find_attribute_pois function only in that case.
There was a problem hiding this comment.
The check is done in the function itself, adding it here would only make this function harder to read and would bring no benefit IMO.
Description
Expose functions from
els_dodgerto be able to make a single pass over all the forms. This improves the performance a bit, although not as much as we would have expected.Fixes #235.
The following are flame graphs captured by running
eflame:apply(els_indexer, index_dir, ["src"]).and then generating the SVG with the following command:Before
After