Opaque Mode #1079

rgrinberg · 2018-08-01T08:47:23Z

Implement #1058

For now, this is just a cleaned up version of my WIP. What remains is:

Implement better user customization. Env & Stanza level. I'm hoping that resolving [PROPOSAL] Env Defaults #1073 first will address this.
Cleaning up some duplication. I have a few hard coded profile = "dev" in some places for example.
Choosing good names. I know that @diml has suggested cross_module_inlining, but IMHO the effect that this has on compilation speed isn't all that intuitive for users.
Update the documentation

ghost · 2018-08-01T10:29:37Z

What is the rationale for adding an opaque field in the library stanza? Is it possible that one would want to set in stone that cross module inlining is always disabled for a library?

rgrinberg · 2018-08-01T10:31:54Z

If the plan is to add the option to env, then I think it makes sense from a consistency point of view to add the option to individual stanzas. I agree with your point from the other thread that anything that can be set in a stanza shouldn't necessarily exist in env. But I think the converse should always hold.

ghost · 2018-08-01T11:08:07Z

Alright, I don't have a strong opinion on this question. I'm happy to go ahead with this.

For the name, it feels like it should be something like (compilation_mode <fast_exe|fast_build>). That should be intuitive.

BTW, it feels unnecessarily painful to add such an option through the Lib module, what about we regroup all the options that are used to build the library itself in a single field that is shared between Lib.t and Lib.Info.t? Or alternatively, we can add a field info : Info.t to Lib.t and remove the duplicated fields.

rgrinberg · 2018-08-01T11:10:22Z

BTW, it feels unnecessarily painful to add such an option through the Lib module, what about we regroup all the options that are used to build the library itself in a single field that is shared between Lib.t and Lib.Info.t? Or alternatively, we can add a field info : Info.t to Lib.t and remove the duplicated fields.

Yeah, that would be fine. I'll try doing this separately first.

jberdine · 2018-08-01T11:40:16Z

For the name, it feels like it should be something like `(compilation_mode <fast_exe|fast_build>)`. That should be intuitive.

Just my 2 cents on terminology: `(compilation_mode <fast_exe|fast_build>)` seems too general a name for such a specific compiler flag. The fast_exe vs fast_build distinction is basically the same as the dev vs release profiles. Also, perhaps I'm odd, but I generally start with the compiler man page to find the options I need, and then hunt through the build system docs to figure out how to get it to pass them. From that perspective, this proposal isn't a very discoverable name.

rgrinberg · 2018-08-01T11:59:47Z

seems too general a name for such a specific
compiler flag. The fast_exe vs fast_build distinction is basically the
same as the dev vs release profiles

Well, we might actually put more options under this umbrella name later. In which case, we'd of course need a proper cross_module_inlining option.

This seems like one of those situations where we can't really win with the names. We can have a name that reflects what is the source in the compiler (opaque), a name that reflects what the option will actually do (cross_module_inlining), or a name that will explain to the user why they might want to toggle this option (compilation_mode).

ghost · 2018-08-01T12:55:24Z

Note that yet another possibility would be to analyze the compilation flags and act accordingly. For instance if we see -opaque in the list of flags then we set the opaque mode. That would have the advantage that you only need to know the compiler flags and we don't need to invent a new name in Dune. However, it is hard to reliably analyze a command line as we don't know what options take parameters.

If we expose the functionality at the dune level via a library field, it seems to me that we should abstract away from the low-level details.

rgrinberg · 2018-08-01T13:54:29Z

src/inline_tests.ml

@@ -233,6 +233,7 @@ include Sub_system.Register_end_point(
          ~scope
          ~dir:inline_test_dir
          ~modules
+          ~opaque:true


Not entirely sure that this is correct, but it seems like we'd want the tests to compiler as fast as possible. @diml?

It's not going to make much difference as this context is only used to build the test runner which contains the entry point and so is not used by anything else. Given that we should stick to the default, i.e. opaque=false.

rgrinberg · 2018-08-01T14:03:08Z

Note that yet another possibility would be to analyze the compilation flags and act accordingly. For instance if we see -opaque in the list of flags then we set the opaque mode. That would have the advantage that you only need to know the compiler flags and we don't need to invent a new name in Dune. However, it is hard to reliably analyze a command line as we don't know what options take parameters.

I'd prefer not to do this as I agree that this isn't as straight forward as it may seem initially. We'd have to do the same trade off that we'd do in menhir and resort to only statically provided options. Im menhir, that seems inevitable, but let's try to avoid this necessary evil here. It would also have the small disadvantage of not being able to disable as easily anymore. Removing something from a set of flags can't be done reliably, and the user would have to set the flags from scratch.

If we expose the functionality at the dune level via a library field, it seems to me that we should abstract away from the low-level details.

As I mentioned before, this would also give us the freedom to add additional meaning to this option in the future.

emillon · 2018-08-01T16:07:19Z

If we don't want to analyze the arguments too hard, maybe we do the following:

add an option that enables -opaque and does the right thing regarding build rules (I agree that naming is hard)
leave flags alone: it is possible for a user to pass -opaque there but they won't have the benefits of the rule improvement
add a separate linting rule/command that warns when -opaque is passed directly in flags (we can probably live with false positives & false negatives here)

This approach can also be used if we want to provide a DSL to express warning arguments, for example ((warnings :standard (partial-match :fatal) unused-rec)).

rgrinberg · 2018-08-01T17:12:02Z

leave flags alone: it is possible for a user to pass -opaque there but they won't have the benefits of the rule improvement

What's the use case for the user ever using -opaque without making use of the better build rules? If there's a valid use case for this then I agree, otherwise if anything we should just error if the user tries to pass this manually.

rgrinberg · 2018-08-02T07:17:42Z

src/super_context.mli

@@ -197,6 +197,8 @@ module Libs : sig
      all the files with extension [ext] of libraries [libs]. *)
  val file_deps : t -> Lib.L.t -> ext:string -> Path.t list

+  val file_deps_with_exts : t -> (Lib.t * string) list -> Path.t list


A suggestion for a better name/implementation would be appreciated. I could also implement file_deps on top of this function, but don't see the point of making the code slower for no reason.

The name is not too much of a problem, but I agree that duplicating is not ideal.

Fortunately, you can extract the body of List.map into private function val file_deps_for_lib : Lib.t -> ext:string -> Build_system.t -> Path.t. That way, the two functions file_deps & file_deps_with_exts become one-liners.

ghost · 2018-08-02T08:13:13Z

FTR, since dependencies can be specified dynamically, we could take into account -opaque flags that are computed dynamically

rgrinberg · 2018-08-02T08:22:01Z

FTR, since dependencies can be specified dynamically, we could take into account -opaque flags that are computed dynamically

When we calculate the includes, don't we have to know whether the library is opaque statically?

ghost · 2018-08-02T08:23:05Z

I believe the include directories won't change since we still need them for the cmi files

rgrinberg · 2018-08-02T08:27:48Z

Okay, I see. Btw, I referred to Compilation_context.Includes.t. Which also introduce glob dependencies along with the flags (*.cmi vs *.cmi and .cmx). I suppose we could just change the type of this includes field to introduce dynamic dependencies

rgrinberg · 2018-08-02T09:36:07Z

Another issue with inferring this property from -opaque is that we might allow users to customize flags per module in the future. It would be much more complicated to support -opaque on a per module basis I think.

rgrinberg · 2018-08-02T10:04:50Z

@jberdine @emillon @diml I honestly have a hard time seeing the need for a fine grained customization option for this flag btw. I see 2 use cases:

You would like to use optimized binaries in development (for benchmarking for example). In this case, I think that a separate benchmarking profile/context would serve the user best.
You'd like to ship the unoptimized object files in release because the optimization doesn't do much for example. But then users can't really make use of this fact to speed up their builds, since we don't detect "opacity" for external libs.

In summary, I think the benefit of this option is miniscule.

ghost · 2018-08-02T10:42:01Z

I agree. That's why I was surprised it ended up in library stanzas.

rgrinberg · 2018-08-02T10:47:32Z

I agree. That's why I was surprised it ended up in library stanzas.

Yeah, although I think it's just as marginally useful in the env stanza. My arguments for adding it to the env stanza are purely from a consistency stand point with env. But now that I'm thinking that this option is barely useful on any level customization, we should wait until users demand it before adding it.

Also note that it's no longer in the Library stanza. We could remove this option from the Lib.t, since we can simply infer what it's going to be based on the profile and on whether the library is internal/external. Do you think we should make this simplification?

ghost · 2018-08-02T10:55:07Z

I think so yes. It seems better to me if the implementation is close to the high-level design

rgrinberg · 2018-08-02T10:59:13Z

Okay, I will remove it from Lib.t. Should keep things nice and simple (and not delay the release)

emillon · 2018-08-02T11:05:27Z

I agree. The knob we have for fast compilation vs fast binary is the profile so it makes sense to support it only there.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg · 2018-08-02T11:28:08Z

@diml okay, I removed all mentions from opaque in Lib.t

@emillon ready for review. Have a look at SC.file_deps_with_exts. I believe that this is currently the most problematic part.

emillon · 2018-08-02T12:34:22Z

Do you have some benchmarks to see if it's actually faster?

emillon · 2018-08-02T12:44:06Z

src/super_context.mli

@@ -197,6 +197,8 @@ module Libs : sig
      all the files with extension [ext] of libraries [libs]. *)
  val file_deps : t -> Lib.L.t -> ext:string -> Path.t list

+  val file_deps_with_exts : t -> (Lib.t * string) list -> Path.t list


The name is not too much of a problem, but I agree that duplicating is not ideal.

Fortunately, you can extract the body of List.map into private function val file_deps_for_lib : Lib.t -> ext:string -> Build_system.t -> Path.t. That way, the two functions file_deps & file_deps_with_exts become one-liners.

emillon · 2018-08-02T12:47:37Z

src/module_compilation.ml

@@ -58,13 +58,14 @@ let build_cm cctx ?sandbox ?(dynlink=true) ~dep_graphs ~cm_kind (m : Module.t) =
        | Cmi | Cmo -> other_targets
      in
      let dep_graph = Ml_kind.Dict.get dep_graphs ml_kind in
+      let opaque = CC.opaque cctx && ctx.version >= (4, 03, 0) in


I think that this kind of test should be done in Context, like Context.supports_opaque_option. There's a similar code path for keep_locs, if you agree with that I can extract the relevant functions in a separate PR.

Yeah, feel free to do it post this PR. I don't have a strong preference where the check should be done but I suppose that diml should have a loc b/c I just kept things as is.

Actually, this made me realize that we should turn off the option everywhere altogether if the OCaml version is below 4.03.0. This is not the only place we should be doing this.

rgrinberg · 2018-08-02T13:10:59Z

Do you have some benchmarks to see if it's actually faster?

Yup. I've noticed an a speed up on on clean builds for cohttp-lwt-unix for example. It's a bit hard to test apples to apples here b/c what we want is to just build a big binary, since that should reduce building non native dependencies. Let me try and setup another little test.

By the way, the certainty in the speed up comes more from observing the rules themselves. There are far less inter cmx dependencies which should result in more paralallization and less recompilation.

Subsequently, use it as a flag when calculating rules and includes Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Only use Compilation_context.t for controlling this Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg · 2018-08-02T13:31:55Z

@emillon have another look

emillon

Great, thanks. Some figures would be nice but as you say it's hard to imagine how it could make things worse.

emillon · 2018-08-02T13:41:02Z

Oh and don't forget to update the changelog!

rgrinberg · 2018-08-02T13:58:23Z

Here are some numbers:

$ rm -rf _build && time dune-dev build cohttp-lwt-unix.install
dune-dev build cohttp-lwt-unix.install  7.57s user 3.98s system 230% cpu 5.008 total
$ rm -rf _build && time dune-dev build cohttp-lwt-unix.install
dune-dev build cohttp-lwt-unix.install  7.61s user 3.98s system 265% cpu 4.363 total

The second run is with -opaque.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

@rgrinberg

CHANGES: - Fix lookup of command line specified files when `--root` is given. Previously, passing in `--root` in conjunction with `--workspace` or `--config` would not work correctly (ocaml/dune#997, @rgrinberg) - Add support for customizing env nodes in workspace files. The `env` stanza is now allowed in toplevel position in the workspace file, or for individual contexts. This feature requires `(dune lang 1.1)` (ocaml/dune#1038, @rgrinberg) - Add `enabled_if` field for aliases and tests. This field controls whether the test will be ran using a boolean expression language. (ocaml/dune#819, @rgrinberg) - Make `name`, `names` fields optional when a `public_name`, `public_names` field is provided. (ocaml/dune#1041, fix ocaml/dune#1000, @rgrinberg) - Interpret `X` in `--libdir X` as relative to `PREFIX` when `X` is relative (ocaml/dune#1072, fix ocaml/dune#1070, @diml) - Add support for multi directory libraries by writing `(include_subdirs unqualified)` (ocaml/dune#1034, @diml) - Add `(staged_pps ...)` to support staged ppx rewriters such as ones using the OCaml typer like `ppx_import` (ocaml/dune#1080, fix ocaml/dune#193, @diml) - Use `-opaque` in the `dev` profile. This option trades off binary quality for compilation speed when compiling .cmx files. (ocaml/dune#1079, fix ocaml/dune#1058, @rgrinberg) - Fix placeholders in `dune subst` documentation (ocaml/dune#1090, @emillon, thanks @trefis for the bug report) - Add locations to errors when a missing binary in PATH comes from a dune file (ocaml/dune#1096, fixes ocaml/dune#1095, @rgrinberg)

rgrinberg requested review from emillon and a user August 1, 2018 08:47

rgrinberg mentioned this pull request Aug 1, 2018

Move info to lib #1082

Merged

rgrinberg force-pushed the opaque branch from 25e4d79 to 4ad8f57 Compare August 1, 2018 13:47

rgrinberg commented Aug 1, 2018

View reviewed changes

rgrinberg commented Aug 2, 2018

View reviewed changes

Hard code opaque mode

8a03e70

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg force-pushed the opaque branch from 6cfd608 to 0638a77 Compare August 2, 2018 11:26

emillon reviewed Aug 2, 2018

View reviewed changes

rgrinberg added 4 commits August 2, 2018 15:26

Make opaque information available to compilation context

b394896

Subsequently, use it as a flag when calculating rules and includes Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Fix opaque calculation for includes

e09bbfc

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Remove opaque from Lib.t

9f43a88

Only use Compilation_context.t for controlling this Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Make file_deps and file_deps_with_exts share code

bde4943

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg force-pushed the opaque branch from c451345 to b51050e Compare August 2, 2018 13:29

Move 4.03 minium version check for opaque to top level

1597b61

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg force-pushed the opaque branch from b51050e to 1597b61 Compare August 2, 2018 13:30

emillon approved these changes Aug 2, 2018

View reviewed changes

rgrinberg changed the title ~~Opaque Mode [WIP]~~ Opaque Mode Aug 2, 2018

Update CHANGELOG for -opaque by in dev

4cbd698

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg merged commit 7132e6e into ocaml:master Aug 2, 2018

rgrinberg deleted the opaque branch August 2, 2018 14:30

This was referenced Aug 3, 2018

Extract predicates for compiler versions #1091

Merged

Support -opaque #497

Closed

rgrinberg mentioned this pull request Aug 6, 2018

[new release] dune (1.1.0) ocaml/opam-repository#12442

Merged

Opaque Mode #1079

Opaque Mode #1079

Conversation

rgrinberg commented Aug 1, 2018 • edited by ghost Loading

ghost commented Aug 1, 2018

rgrinberg commented Aug 1, 2018

ghost commented Aug 1, 2018

rgrinberg commented Aug 1, 2018

jberdine commented Aug 1, 2018 via email

rgrinberg commented Aug 1, 2018

ghost commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Aug 1, 2018

emillon commented Aug 1, 2018

rgrinberg commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 2, 2018

rgrinberg commented Aug 2, 2018

ghost commented Aug 2, 2018

rgrinberg commented Aug 2, 2018

rgrinberg commented Aug 2, 2018

rgrinberg commented Aug 2, 2018

ghost commented Aug 2, 2018

rgrinberg commented Aug 2, 2018 • edited Loading

ghost commented Aug 2, 2018

rgrinberg commented Aug 2, 2018

emillon commented Aug 2, 2018

rgrinberg commented Aug 2, 2018

emillon commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Aug 2, 2018

rgrinberg commented Aug 2, 2018

emillon left a comment

Choose a reason for hiding this comment

emillon commented Aug 2, 2018

rgrinberg commented Aug 2, 2018

rgrinberg commented Aug 1, 2018 •

edited by ghost

Loading

rgrinberg commented Aug 2, 2018 •

edited

Loading