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

Subst.signature: only call cleanup_types once. #2261

Merged
merged 4 commits into from Feb 25, 2019

Conversation

Projects
None yet
4 participants
@trefis
Copy link
Contributor

commented Feb 21, 2019

Fixes MPR#7929.

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

The invariant that you are trying to enforce is that every call to typexp is followed by a call to cleanup_types at the right scope level to catch all equi-recursive/cyclic occurrences of a type (types are cyclic graphs): the call must happen late enough, but it's a bug if it does not happen at all.

At first I thought that the invariant in your code, doing a worker-wrapper transform on several functions in the module, is that worker functions only call worker functions, and wrappers always cleanup. This is correct if only the wrappers are exposed through the module interface.

Unfortunately, it is not clear from the code of the module that the transformation was performed correctly. For example, label_declaration calls typexp without cleanup, although its name looks like a wrapper name; in fact it is a worker (which is correct as it is not exposed in the module interface).

You could kill the problem instead by using the type system in your favor.

module Recursion : sig
  type scope
  val save_desc : scope -> typexpr -> ty_desc -> unit
  val with_cleanup : (scope -> 'a) -> 'a
end

(* all declarations, with a scope parameter *)
let typexp scope ... = ...
let type_declaration' scope decl = ..

[...]

(* then only the wrappers at the end *)
let type_declaration = Recursion.with_cleanup type_declaration
@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

If we have a gentlemen's agreement that such a rewrite would be accepted in 4.08 I'd be happy to implement it.

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Deal.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

I pushed a slightly broader version of your suggestion which covers uses of save_desc in Ctype as well.
I didn't go with Recursion for the module name because "meh", but my choice is just as "meh", so I wouldn't mind changing it to anything else (including Recursion).

As it turns out, while I disagree with

Unfortunately, it is not clear from the code of the module that the transformation was performed correctly

it turns out that the change caught calls to typexp made from compose, which were never cleaned up. I changed them for calls to type_expr.

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

It's very nice that you generalized this to all uses of cleanup_types. I agree the naming could get refined slightly, though:

  • for Clean_copy, maybe just Copy? Or Traverse?
  • scope itself is a bit weird in Ctype which already has other notions of expansion_scope and others laying around (there is one point where you start using cleanup_scope for clarity). The type could be copy_state or copy_scope, and the name could match.
  • with_cleanup made sense at first, but now that you have completely hidden cleanup it is weird. I would just use run, or maybe with_scope or with_state.

Personally I like the idiom

with_resource @@ fun resource ->
<code at the same indentation level

but it looks like you prefer plain parentheses, and that is also fine with me.

I wonder if there are nested calls to with_cleanup that happen in the compiler. One way to test against this (probably a bug) would be to keep global mutable state in the module to catch nested calls. Could you maybe try a build of the whole compiler distribution with this, and some logic to fail noisily if nested calls do happen?

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

(Re. nested calls: my gut feeling would be that we could/should keep the dynamic instrumentation to forbid them, unless it turns out that there are presently used for some sensible reason.)

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

  • for Clean_copy, maybe just Copy? Or Traverse?

Copy was actually my initial version, but I find it worse in some ways...

  • scope itself is a bit weird in Ctype which already has other notions of expansion_scope and others laying around (there is one point where you start using cleanup_scope for clarity). The type could be copy_state or copy_scope, and the name could match.
  • with_cleanup made sense at first, but now that you have completely hidden cleanup it is weird. I would just use run, or maybe with_scope or with_state.

I agree with all of that. I'll make a change in that direction.

but it looks like you prefer plain parentheses, and that is also fine with me.

It wasn't always the case, but I indeed prefer that now. Thank you.


About nested calls: there definitely were obvious nested/overlapping calls to cleanup_types before, which I think were safe to remove so I did (cf. Ctype.instance_poly').
It could be the case that there are now non-obvious nested calls to with_cleanup, though I doubt it, and I would have to think to know if they make sense or not, if it's safe to remove them or not. But that feels like it's outside of the scope of a bugfix.
So I'd suggest stopping here for now and picking the work up again on trunk if you want.

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

So I'd suggest stopping here for now and picking the work up again on trunk if you want.

That works for me.

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

If I understand correctly, the scope argument in save_desc and with_cleanup is just a visual clue: nothing in the type system prevents you from using with_cleanup (fun x -> x) to get a forged one, but one is just expected not to do that. This might be mentioned in the interface.

Concerning the approach of delaying the cleanup for a whole signature, rather than just for classes and associated types, this should be semantically correct, but beware that since it increases sharing, you may uncover some bugs, where one modifies a generalized type without instantiating it. Since those are bound to be bugs, uncovering them is a good thing, but this may still bite you.
(Actually I'm not even sure that it will increase sharing, since in all other cases I believe that all "shared" types should have gone through generalization/instantiation/generalization, so that they are no longer shared, so I'm just talking of a remote possibility.)

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

If I understand correctly, the scope argument in save_desc and with_cleanup is just a visual clue: nothing in the type system prevents you from using with_cleanup (fun x -> x) to get a forged one, but one is just expected not to do that. This might be mentioned in the interface.

That is correct. I'll update the doc.

Concerning the approach of delaying the cleanup for a whole signature, rather than just for classes and associated types, this should be semantically correct, but beware that since it increases sharing, you may uncover some bugs, where one modifies a generalized type without instantiating it. Since those are bound to be bugs, uncovering them is a good thing, but this may still bite you.

Indeed.
I did compile a good chunk of janestreet's codebase (which I'll call X from now on) with that patch without any issue, so I think the risk is acceptable.

Actually I'm not even sure that it will increase sharing, since in all other cases I believe that all "shared" types should have gone through generalization/instantiation/generalization, so that they are no longer shared, so I'm just talking of a remote possibility.

Yes, I wanted to verify that so I built X twice: once with 4.07.1 and once with 4.07.1 + this GPR. The aggregated cmi size for X is around 920M, and was reduced by less than 1M with this GPR.

@trefis trefis force-pushed the trefis:subst-clean-once branch from 3b0d73e to ecb093b Feb 22, 2019

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

I updated the code and added a Changes entry.

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Note: given the current implementation of cleanup with a single piece of global mutable state, I think that nested calls are clear bugs (when the inner call finishes and cleans up, it will also mutate the sites of the outer call, before that outer call has completed), so it's probably the right thing to forbid them. (But it's fine to keep it out of this PR.)

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Let's change that then! I'll push a version which removes the global state and passes it around. Should have done that from the start.

@trefis trefis requested a review from gasche Feb 22, 2019

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Other note: we could make with_scope (fun x -> x) illegal by using the ST trick: adding a phantom type parameter to the copy_scope type, that our consumers are parametric over, and

type 'a in_scope = { run : 's . 's scope -> 'a }
val with_scope :  'a in_scope -> 'a

I don't have a strong opinion on whether this should be done or not -- if I was the PR author, I probably wouldn't. It makes the code nominally safer, but also slightly more complicated.

@gasche

gasche approved these changes Feb 22, 2019

Copy link
Member

left a comment

I read the current version of the patch and I think it is good to merge -- although some potential revisions have been mentioned, so maybe @trefis will want to make more changes before that.

Show resolved Hide resolved typing/btype.mli Outdated
Show resolved Hide resolved typing/ctype.ml Outdated

@trefis trefis force-pushed the trefis:subst-clean-once branch from 3f8d3c8 to 4e4fc87 Feb 22, 2019

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

I don't have a strong opinion on whether this should be done or not -- if I was the PR author, I probably wouldn't. It makes the code nominally safer, but also slightly more complicated.

Indeed I won't.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Feel free to merge this (and cherry pick on trunk) if you want.

Otherwise, I'll probably do it on Monday.

@gasche

gasche approved these changes Feb 22, 2019

Copy link
Member

left a comment

(I may or may not have time to merge before Monday.)

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

Actually, I'm not sure that making the state local is a good idea: the problem is not with save_desc, but with the way type nodes are physically modified after using it. For instance, if a node has been made a Tsubst inside a first with_scope, a traversal within a nested with_scope will assume that it was already traversed, and behave wrongly. So a nested use of with_scope is still going to be wrong in most cases and, as @gasche suggested, protecting against this could be a good idea. In theory this should be an assert false, but maybe a warning is sufficient in case the problem was not fatal.

Also, the polymorphism trick mentioned above is only safe in a pure functional language. In ocaml, you can return a closure that is going to use the scope later (without showing it in its type), so this only provides a false sense of security. In my opinion, if we cannot provide full safety, then a visual clue protecting against dumb mistakes is sufficient.

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

@garrigue I agree with your analysis, but I think that making the state local still is an improvement. For example, if we want to make the runtime reentrant in the future, we need to have this state localized anyway. What would be best would be to have the state local and protection against incorrect nested calls (through the re-addition of a tiny amount of global state, or adding a parameter to Tsubst to identity which call made the mutation and recognize mismatches).

But the error-handling logic that we are discussing is subtle, and requires more work -- as you say, we may not want the error to be fatal at first, which ironically makes it much more cumbersome to implement propertly. I believe that it is a good choice of @trefis to leave that for later, and concentrate on the bug-fixing for what should also go in the 4.08 release branch -- the present PR.

Therefore, I think that the current PR is in a good state to be merged in both trunk and 4.08, with further iterations going in trunk only.

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

My idea was rather that one could very easily detect nested calls to with_scope, which in the current code base are almost certain to be bugs (since the API was not reentrant).
If we start building reentrance in, it becomes harder to define what is a bug and what is not.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

I was under the impression that we were going to try and get rid of Tsubst nodes soon anyway (e.g. its an item in TODO.md), so maybe we shouldn't worry too much about the form of the current patch.

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

And if we get rid of Tsubst we also get rid of cleanup_types, so there is no point in trying to make it reentrant :)
This said, being an item in a TODO doesn't mean that it will be done immediately.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Therefore, I think that the current PR is in a good state to be merged in both trunk and 4.08, with further iterations going in trunk only.

I will indeed merge this PR as is.
Which doesn't mean that the conversation can't continue if people want to discuss further improvements for trunk.

@trefis trefis merged commit c64e8a8 into ocaml:4.08 Feb 25, 2019

2 checks passed

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

@trefis trefis deleted the trefis:subst-clean-once branch Feb 25, 2019

trefis added a commit that referenced this pull request Feb 25, 2019

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

This has now been cherry-picked on trunk.

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.