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

Refactor driver/compile_common #2213

Merged
merged 8 commits into from Jan 5, 2019

Conversation

Projects
None yet
3 participants
@gasche
Copy link
Member

commented Jan 1, 2019

I was working on a separate PR that would add a new field to the Compile_common.info structure, and I realized that there were small aspects of the API that I don't like much. Given that it was only added in the development version, it is time to consider small refactorings before the code reaches the end-users.

I would be interested in particular in the opinion of @Drup and @sliquister (if they have time) who worked on the original code.

This PR is best looked at commit-per-commit, and it may be the case that you like some of them and reject some others (I'm willing to remove some parts). All the commits should be exactly semantics-preserving.

I think that "consistently use _ as word separator", "rename init_path -> native", "whitespace refactoring" and "store 'native' in 'info'" are clear wins. I believe that removing ppf_dump is a nice-to-have (it was a value relying on a lifetime-limited resource with unclear guarantees on liveness), but it's less clear-cut. The make/init split is the weakest part of the PR, and I'm interested in opinions on the final refactoring.

@Drup

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2019

I'm happy to see some iteration on this new API! I had a quick look through the changes, I like all of them except the make/init split.

I don't mind the idea of separating info-recording and side effects, but I really dislike having the env field mutable. I'm also not convinced it's really safer that way. People could just forget to call init, and it really should be an Env.t option. What's your goal exactly here ? Would it be solved by a reinit : info -> info function ?

Show resolved Hide resolved driver/compile.ml Outdated
Show resolved Hide resolved driver/compile_common.mli Outdated
@sliquister

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2019

The changes you declared as clear wins look good to me.

The ppf change has the problem I indicated inline.

I am not sure about the create/init split, because it makes it unclear who's responsible for initializing the global state. And FWIW, module_of_filename is not a pure function (can print warnings), so even at the final state, make_info is not a pure function.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2019

My idea at the beginning was that the make/init split was probably necessary to generalize info as an input type to more functions, to be able to innocuously create info values and have them initialized deeper in the system. But that doesn't really fly, and I will get rid of the commit, as well as the ppf_dump removal for the reason @sliquister pointed out (thanks!).

On the other hand, I think that the make/init split is attacking a real problem with the current API, which is the way the creation of an info value is tied to a time-limited environment side-effect. (I'm not talking about the formatter's lifetime here, which if broken is easy to detect, but about the environment state.) It is highly unsafe to create several info values and use them in parallel, for example; this violates natural assumptions about reification into the info value. I don't have the time to do any better than rolling back my splitting commit, but hopefully in 2019 we will remove more ambient state from the compiler implementation.

@sliquister

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2019

Ok. Interleaved compilations is one problem, but even sequential independent compilations don't work as there's global state that can't be reset without patching the compiler (there's state in asmcomp about linking that's not reset (or there was a year or two ago at least), plus the entirety of command line parsing).

@gasche gasche force-pushed the gasche:refactor-compile-common branch from 218ee23 to 9b3fa9f Jan 4, 2019

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

I made the suggested changes. Thanks!

(I did think about a mutable : Env.t option or even a mutable : Env.t Lazy.t, but given that we can never get a clean init/make anyway I gave up for now.)

@sliquister: I'm thinking of turning the Formatter parameter of init into a labelled argument (~ppf_dump) for consistency, would that work for you?

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

I made a last change (last commit), which is to turn Compile_common.init : ... -> info into Compile_common.with_info : ... -> (info -> 'a) -> 'a. Usually I'm not fond of the more complex types of continuation-passing APIs, but in that case this nicely encapsulates the idea that info files, as they are bound to some global state, have a limited validity. In particular, it lets me hide the ppf_dump allocation within this call -- expecting an additional dump_ext parameter that determines the extension (and thus the name) of the dump file.

@Drup

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

The issue with with_info is that the underlying implementation doesn't match the API. The compiler let lot's of pieces of state and resources around that don't really get reset if you call with_info again.

That being said, that API definitely seems like a good goal, so I'm not really opposed to it. I guess I would prefer if there were explicit comments about the fact that this definitely doesn't encapsulate the complete state of the compiler's pipeline.

@gasche gasche force-pushed the gasche:refactor-compile-common branch 2 times, most recently from 3df1d60 to 95bdcd3 Jan 5, 2019

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

I just added the following to the documentation comment:

(** [with_info ~native ~tool_name ~source_file ~output_prefix ~dump_ext k]
   invokes its continuation [k] with an [info] structure built from
   its input, after initializing various global variables. This info
   structure and the initialized global state are not valid anymore
   after the continuation returns.

+   Due to current implementation limitations in the compiler, it is
+   unsafe to try to compile several distinct compilation units by
+   calling [with_info] several times.
*)
Compile_common: turn `init` into continuation-passing `with_info`
I think this clarifies the fact that the `info` value is a resource that
has a limited lifetime. The new API lets us create (and close) the ppf_dump
directly from `Compile_common.with_info`, simplifying the API for user.
@Drup

Drup approved these changes Jan 5, 2019

Copy link
Contributor

left a comment

Looks good!

@gasche gasche merged commit d9ee956 into ocaml:trunk Jan 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.