-
Notifications
You must be signed in to change notification settings - Fork 469
Remove cmij cache #6192
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
Remove cmij cache #6192
Conversation
a9e2633
to
98c18b1
Compare
98c18b1
to
216020d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great.
In terms of eliminating unneeded code this is probably all.
However if curious, one can have a very quick look at the output of make dce
. It's quite verbose as we haven't really done dead code elimination on the entire compiler.
@@ -22,44 +22,7 @@ | |||
* along with this program; if not, write to the Free Software | |||
* Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. *) | |||
|
|||
#ifdef RELEASE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@@ -27,7 +27,7 @@ let init_path () = | |||
let exp_dirs = | |||
List.map (Misc.expand_directory Config.standard_library) dirs | |||
in | |||
Config.load_path := List.rev_append exp_dirs []; | |||
Config.load_path := List.rev_append exp_dirs [Config.standard_library]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Presumably this must have been there a long time ago.
To double check: this should have no effect on pervasives I imagine. Just in case the fact that we build 2 versions of pervasives for uncurried and legacy mode is relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, the CMI files (that were formerly loaded from the cache) are not found.
@@ -354,280 +352,340 @@ lib/js/sys.js | |||
lib/js/uchar.js | |||
lib/minisocket.js | |||
lib/ocaml/arg.cmi | |||
lib/ocaml/arg.cmj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any other consequences to the created package?
I guess you've tested installing the package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No other consequences. I have tested the package.
@@ -46,7 +46,7 @@ if (isCheckMode) { | |||
} | |||
|
|||
function getFilesAddedByCI() { | |||
const platforms = ["darwin", "darwinarm64", "linux", "win32"]; | |||
const platforms = ["darwin", "darwinarm64", "linux", "linuxarm64", "win32"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is a tangentially related fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I noticed this when regenerating the artifact list.
@@ -1468,13 +1468,6 @@ function main() { | |||
cwd: jscompDir, | |||
stdio: [0, 1, 2], | |||
}); | |||
if (!isPlayground) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice simplification too.
Definitely interested in looking at eliminating dead code across the compiler, but not in the scope of this PR. |
In my tests on my M1 MacBook, I did not see any speed difference when compiling a large project with the cmij cache removed.