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.

Sign up for GitHub

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

Jump to bottom

Split calls to ocamldep #486

Merged
4 commits merged into from
Feb 6, 2018
Merged

Split calls to ocamldep #486

4 commits merged into from
Feb 6, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 5, 2018

Instead of doing a single call to ocamldep, do one per file. This will allow to support menhir --infer (#305).

@ghost ghost requested a review from rgrinberg February 5, 2018 19:27
Split calls to ocamldep
4361cc6 
Instead of doing a single call to ocamldep, do one per file. This is
needed to support "menhir --infer".

This should also make compilation go further when there are files with
syntax errors.
@ghost ghost force-pushed the split-ocamldep branch from 90a3399 to 4361cc6 Compare February 5, 2018 19:27
@rgrinberg
Copy link
Member

rgrinberg commented Feb 5, 2018 •
edited
Loading

The cram tests disagree in CI:

--- test/blackbox-tests/test-cases/js_of_ocaml/run.t	2018-02-05 19:35:38.537609760 +0000
+++ test/blackbox-tests/test-cases/js_of_ocaml/run.t.corrected	2018-02-05 19:35:44.809450344 +0000
@@ -8,8 +8,10 @@
            ppx bin/technologic.pp.ml
            ppx bin/z.pp.ml
       ocamlopt lib/x__.{cmx,o}
-      ocamldep lib/x.depends.ocamldep-output
-      ocamldep bin/technologic.depends.ocamldep-output
+      ocamldep lib/x.pp.ml.d
+      ocamldep lib/y.pp.ml.d
+      ocamldep bin/technologic.pp.ml.d
+      ocamldep bin/z.pp.ml.d

Curious about why is this change desired. Doesn't this slow builds by invoking an external command potentially a lot more often?

The interface looks much better. build_exe should be a lot easier to use.

rgrinberg
rgrinberg reviewed Feb 5, 2018
View reviewed changes
src/ordered_set_lang.ml Outdated
String_map.find_exn (f fn) files_contents ~string_of_key:(sprintf "%S")
~desc:(fun _ -> "<filename to s-expression>")
) ~f:(fun s -> f (String_with_vars.t s))
parse_general (Option.value_exn (String_map.find (f fn) files_contents))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String_map.find (f fn) files_contents returning None was also impossible before this change as well? Could you remind me why that is?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is only called from Super_context.expand_and_eval_set where it's easy to check the correctness. I hesitated between doing this and upgrading this code to Sexp.code_error but maybe I should, just for uniformity

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/module_compilation.ml Outdated
| Cmi | Cmo -> [Module.cm_file m ~dir Cmi]
| Cmx -> [Module.cm_file m ~dir Cmi; Module.cm_file m ~dir Cmx])))
(Ocamldep.Dep_graph.deps_of dep_graph m >>^ fun deps ->
List.concat_map deps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An accidental space snuck in here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops

@ghost
Copy link
Author

ghost commented Feb 6, 2018 •
edited by ghost
Loading

I'll check the performance implications. The motivations are:

  • it's the only I can think of to support menhir --infer, i.e. we need to compile some .cmi files before we can generate some other .ml files
  • it might improve parallelism in some cases, since we'll be able to start building some modules before all the .ml are generated
  • it seems that sometimes users like to be able to start building with .ml files that are not finished and have syntax errors, this will allow it
  • incremental build will be slightly faster, since only .ml files that have changed will be passed through ocamldep. The impact will be bigger when we support ppx_import

@ghost
Copy link
Author

ghost commented Feb 6, 2018

I tried to build jenga from scratch with -j1 and -j20. It is slightly slower indeed:

master -j1

real	2m58.795s
user	2m16.904s
sys	0m25.476s

split -j1

real	3m7.211s
user	2m18.196s
sys	0m30.108s

master -j20

real	0m49.544s
user	3m36.376s
sys	0m33.088s

split -j20

real	0m52.668s
user	3m32.132s
sys	0m36.928s

Though I'm still in favour of this patch since it allows to support menhir --infer. Additionally, if we implement something like #193 (comment), then this won't matter anyway and the new API is more friendly to such a change.

@ghost
Copy link
Author

ghost commented Feb 6, 2018

I'm merging this as the slowdown is relatively small, we have a way to improve things and I need this PR for another one I'm preparing.

@ghost
Copy link
Author

ghost commented Feb 6, 2018

Well, I'll merge it once node is installed and I can re-generate the js_of_ocaml tests...

@ghost ghost merged commit 80c0bfc into master Feb 6, 2018
@rgrinberg
Copy link
Member

rgrinberg commented Feb 6, 2018 via email

@ghost ghost deleted the split-ocamldep branch February 7, 2018 14:47
@dra27 dra27 mentioned this pull request Mar 3, 2018
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@rgrinberg