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
"include" with module aliases #7056
Comments
Comment author: @garrigue How huge the blow-up ? Of course, module aliases require no runtime information, so we could change the internal representation. However, this complicates the computation of offsets, which is easy to get wrong, so I would first like to know if this really matters. |
Comment author: @mshinwell I'm almost certain it wasn't one word per module. It looked like it was redefining the entire structure, starting at (something like) Core.Std and going all the way down. I only have a dump of the unsimplified flambda now. Here is an excerpt:
I'm not sure if this makes any difference, but flambda uses the bytecode compilation method for modules (all of the transl_store* functions have been removed). |
Comment author: @mshinwell (Oh, and the blowup was enormous. The file took about ten minutes to compile on a fast machine.) |
Comment author: @mshinwell (For information: in the excerpt above, Core_kernel__Validate.camlCore_kernel__Validate is the symbol assigned to the module block of Core_kernel.Validate in the .cmx file being imported.) |
Comment author: @sliquister Mark, are you sure the code is not instead: include (Core.Std : module type of Core.Std) or something similar? Because 'module type of' expands all aliases, which creates big typing environments and most likely the corresponding big ast after the module coercions are explicit. |
Comment author: @mshinwell I will check again, but I'm pretty sure "module type of" was not involved. |
Comment author: @garrigue Note that the coercion need not be on the include itself; doing it in the mli for instance may have the same impact. |
Comment author: @lpw25 IIRC module aliases still occupy a position in the module at runtime, even though this position always contains unit. It is not clear to me why this is the case. Couldn't they just be excluded from the runtime module? |
Comment author: @garrigue Yes, but that wouldn't solve the problem described here. There are 2 reasons I didn't to try to get rid of these few words: (1) this makes computing offsets more complex, and (2) this would require some changes in the way coercions are built. Both of those are tricky. But this could certainly be done. |
Comment author: @mshinwell Ah yes, the original code had "include module type of Foo.Std" in the .mli ... |
Comment author: @garrigue If this is the case, then the right way to write this is going to be include (module type of struct include Foo.Std) which ensures that you really get aliases. |
Comment author: @sliquister (module type of struct include X end) doesn't ensure one gets aliases. They are expanded unconditionally (except for the bug in remove_aliases where it doesn't traverse functor types), which is a problem because it means [include (X : module type of struct include X end)] forces any compilation unit pointed to by an alias in the type of X to be linked in. |
Comment author: @garrigue @sliquister: Maybe we could add a "module type of this M", to mean that we keep exactly the original strengthened signature. |
Comment author: @sliquister The case where the expansion added unneeded link time dependencies for me was not the one reported here, but was of the following form:
I would gladly not use That being said, why not change If you use
If you're going to write a whole new implementation, it's a fraction of the code to also write an interface. [1]
Removing components from modules instead of module types:
Or as you say, a syntax for the actual type of a module:
(although I think the current construct should be |
Comment author: @lpw25
I would much rather see "module type of" changed to mean the My arguments for changing from the current semantics to the
Essentially, we currently have a semantics designed to support an At Jane Street we have to deal with a new issue based on "module |
Comment author: @garrigue lpw25, I understand your point. Concerning the problem with include, what about adding a new syntax "include module M", |
Comment author: @sliquister For what it's worth, I happened to notice about 10 big cmts in our tree today (from 50MB to 190MB), and in all cases, the source file contains 'module type of something big' where the alias expansion is just an undesirable side effect. |
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
I think this is now fixed. Module aliases no longer take up space in a module and |
Original bug ID: 7056
Reporter: @mshinwell
Assigned to: @garrigue
Status: assigned (set by @mshinwell on 2015-11-24T13:46:20Z)
Resolution: open
Priority: normal
Severity: minor
Version: 4.03.0+dev / +beta1
Category: typing
Monitored by: @gasche @diml @hcarty @yakobowski @alainfrisch
Bug description
It seems that if you have something like:
std.ml
test.ml
then the generated code for test.ml includes the creation of module blocks that copy (presumably unit) values out of the dummy fields created for A in std.ml, even though A is an alias. I've seen this lead to a huge blowup in code when someone did something along the lines of "include Core.Std". Could we suppress this behaviour?
The text was updated successfully, but these errors were encountered: