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

MPR#7643, ocamldep: fix nested structures blowup #1377

Merged
merged 4 commits into from Oct 19, 2017

Conversation

Projects
None yet
5 participants
@Octachron
Copy link
Contributor

Octachron commented Sep 28, 2017

MPR#7643:

Currently, ocamldep computation time blows up exponentially in presence of deeply nested structures,
e.g. include (struct include struct … end) or module M = struct module M = struct … end end.

This is due to a duplicated call to Depend.add_structure_binding inside Depend.add_module_binding.
This PR fixes this issue by manually inlining the relevant code and removing the duplicated call.

@Octachron Octachron added the bug label Sep 28, 2017

@Octachron Octachron requested a review from gasche Sep 28, 2017

@Octachron Octachron force-pushed the Octachron:ocamldep_nested_include_fix branch from fbabc0f to 5a77295 Sep 28, 2017

@gasche
Copy link
Member

gasche left a comment

I am not satisfied that I understand the code enough for now (and I hope that the result can be made simpler), see question. Also, add_modtype_binding follows the same structure so I expect that it should be changed as well.

| _ ->
if !Clflags.transparent_modules then add_module bv modl; bound
let n = make_node (snd @@ add_structure_binding bv s) in
if not !Clflags.transparent_modules then add_names (collect_free n);

This comment has been minimized.

@gasche

gasche Sep 28, 2017

Member

Can you remind me when the module M = struct ... end path would at all depend on whether transparent_modules is set or not? I would naively expect that only the module N = Path case is affected, and that as a consequence you could just call add_structure s here.

This comment has been minimized.

@Octachron

Octachron Sep 28, 2017

Author Contributor

You are right that I forgot the modtype case.

Concerning your other question, I struggled a bit to understand why the collect_free is needed at all. Indeed, collect_free collects imported compilation units in submodules; but the compilation units imported within the current file have already been recorded at an earlier stage if transparent_modules is not set.

This if not !Clflags.transparent_modules then add_names (collect_free n); statement is here to record imported compilation units that comes from a map module and need to be only activated when we are not compiling dependencies for map modules . For instance,

(* atlas.ml *)
module A = X 
module B = Y
(*r.ml*)
include Atlas.M

Then, if r.ml is not supposed to be a map file,

ocamldep -map atlas.ml r.ml -modules

yields r.ml: Atlas X Y, compared to

ocamldep -map atlas.ml -as-map r.ml -modules

that outputs simply r.ml: Atlas .

… this behavior may deserve some more comments in the code.

@damiendoligez damiendoligez added this to the 4.06.0 milestone Sep 29, 2017

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Oct 9, 2017

@Octachron : do you plan to revise as requested shortly?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 9, 2017

Note: the current status of the PR is that I reviewed it and agree that it preserves the existing behavior while making the exponential blowup go away (but I still find the code confusing and, to be honest, haven't wrapped by head around @Octachron's explanation). If @Octachron finds himself too busy to work on the PR more, I think it is valuable to merge it as-is.

(Given that the blowup only happens on unnatural code shape, we could perfectly decide to not include the fix in 4.06. On the other hand, it's a fairly safe fix to a non-critical part of the distribution, so I think there is no reason to wait for 4.07.)

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Oct 9, 2017

@xavierleroy , I will try to make the code more understandable (the add_modtype_bind part has been fixed by the second commit) in the begining of the week.

@Octachron Octachron force-pushed the Octachron:ocamldep_nested_include_fix branch from bd71266 to 7990c8f Oct 10, 2017

@Octachron Octachron force-pushed the Octachron:ocamldep_nested_include_fix branch from 7990c8f to e4c8f10 Oct 10, 2017

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Oct 10, 2017

@gasche: I have reworked the code structure to try to make clearer the motivation behind the use of Clflags.transparent_modules. This transparent_modules setting is now only used in two places:

  • add_module_alias to determine if an alias module M = Aliased induces a dependency on Aliased
  • in add_struct_item when including a module to import potentially delayed dependencies
@gasche
Copy link
Member

gasche left a comment

The new code looks simpler indeed, but I haven't reviewed (yet) that the two versions are equivalent.

When trying to read the code as a whole, I find it very frustrating that there is both an addmodule and an add_module function. Could we rename them, for example add_module_path and add_module_expr?

add_parent bv l;
(* If we are in delayed dependencies mode, we delay the dependencies
induced by "Lident s" *)
(if !Clflags.transparent_modules then add_parent else addmodule) bv l;

This comment has been minimized.

@gasche

gasche Oct 10, 2017

Member

The code makes it hard to know which of the two statements may raise Not_found; am I correct that only lookup_map may raise here? If so, could we rewrite the code to take the first statement out of the try .. with block?

This comment has been minimized.

@Octachron

Octachron Oct 10, 2017

Author Contributor

Indeed, fixed.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Oct 10, 2017

I agree that add_module_expr and add_module_path are much clearer than the previous names, fixed.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Oct 19, 2017

@gasche Would you have time to look at this? It's marked for 4.06.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 19, 2017

I like the new code but I don't fully trust myself that I understand the details enough to evaluate its correctness. I won't have more time to review this by the 4.06 deadline, and in fact this is fixing a bug that is very uncommon and not critical. I'll merge in trunk now, not 4.06.

@gasche

gasche approved these changes Oct 19, 2017

@gasche gasche merged commit 8058689 into ocaml:4.06 Oct 19, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 19, 2017

Hm, I clicked "merge" too hastily, the branch was set for 4.06. Will revert from 4.06 and merge in trunk.

gasche added a commit that referenced this pull request Oct 19, 2017

Revert "Merge pull request #1377 from Octachron/ocamldep_nested_inclu…
…de_fix"

(I would like this to be in trunk, but not necessarily 4.06)

This reverts commit 8058689, reversing
changes made to a55915f.
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 19, 2017

In trunk: c904d41
4.06 revert: 5d30640

@vicuna vicuna referenced this pull request Mar 14, 2019

Closed

ocamldep loops #7643

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.