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

Further reduce the size of cmx files (flambda) #1666

Merged
merged 10 commits into from Apr 9, 2018

Conversation

Projects
None yet
4 participants
@xclerc
Copy link
Contributor

xclerc commented Mar 19, 2018

(The last part of #1343, after #1627 and #1665)

Reduces the size of cmx files by computing the set of symbols that can
be reached from the module block, and then only keep in cmx files the
approximations for reachable symbols.

xclerc added some commits Mar 19, 2018

Compute the transitive closure of symbols that can be reached from th…
…e module block, so that we only keep in cmx files the approximations for symbols that can be reached.

@xclerc xclerc changed the title Further reduce the size of cmx files in classic mode (flambda) Further reduce the size of cmx files (flambda) Mar 19, 2018

@xclerc

This comment has been minimized.

Copy link
Contributor Author

xclerc commented Mar 20, 2018

This PR was originally written @fyquah95, and reviewed by @lpw25 and myself.
My understanding is that we hence only need validation of the general approach
(typically from @chambart).

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Mar 21, 2018

@xclerc people wanting to review this pull-request should only look at the last commit right? The other ones are already included in other PRs (such as #1665).
Correct?

@xclerc

This comment has been minimized.

Copy link
Contributor Author

xclerc commented Mar 21, 2018

Indeed, only the last commit (i.e. e3f26ae) is of interest here.
The other ones are from other PRs, but needed for CI.

@chambart
Copy link
Contributor

chambart left a comment

This is quite good. This produce some interesting improvements to cmx file size and compilation time. This also contains some good refactorings.

There might be some errors in the traversal I didn't catch but building any decent codebase should catch any problem there immediately.

Otherwise, I don't think that the testsuite contains any test for classic mode. I don't know what would be the right way to do that, but this might be the right time to add some since there are now some more differences.

let opaque_transient ~compilation_unit ~root_symbol : transient =
let export_id = Export_id.create compilation_unit in
let values =
let map = Export_id.Map.of_list [(export_id, Value_unknown_descr)] in

This comment has been minimized.

@chambart

chambart Apr 6, 2018

Contributor

That's the only use for Value_unknown_descr. Was that really required to introduce a new case for that given that there is another construction (Value_unknown) for representing unknown values.

Otherwise, Map.singleton is the right function to use here. (below too)

| exception Not_found ->
A.value_unresolved (Symbol sym)
else begin
let compilation_unit = (Symbol.compilation_unit sym) in

This comment has been minimized.

@chambart

chambart Apr 6, 2018

Contributor

Some spurious parentheses

xclerc added some commits Apr 6, 2018

@mshinwell mshinwell merged commit 835cad1 into ocaml:trunk Apr 9, 2018

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.