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

--MLish and extraction #3142

Merged
merged 13 commits into from
Dec 8, 2023
Merged

--MLish and extraction #3142

merged 13 commits into from
Dec 8, 2023

Conversation

mtzguido
Copy link
Member

@mtzguido mtzguido commented Dec 8, 2023

This is meant to fix the snapshots diffs observed locally. The problem is that some files in ulib (those that the compiler depends on) are extracted twice: once by the extraction of the compiler and once for the library snapshot. Both of these write to the same file, so any discrepancy between them causes a racy build.

I may be forgetting the details of our discussion some time ago but this dual extraction is a bit problematic, first it's repeated work, and second can have these subtle problems.

This PR makes everything extract with --MLish, which works fine after a fix or two.

What I also don't understand is if there's a reason why we extract with --MLish.. why does extraction care? FWIW I tried also disabling --MLish on extraction everywhere and this also works.

@nikswamy @tahina-pro any opinions?

@mtzguido
Copy link
Member Author

mtzguido commented Dec 8, 2023

FWIW I tried also disabling --MLish on extraction everywhere and this also works.

I think I would like this better. And make --MLish meaningless for extraction.

@nikswamy
Copy link
Collaborator

nikswamy commented Dec 8, 2023

What I also don't understand is if there's a reason why we extract with --MLish.. why does extraction care?

We do not normalize terms for extraction when MLish is on. I think it was intended as a performance benefit for extracting the compiler sources, but maybe it doesn't matter any more.

https://github.com/FStarLang/FStar/blob/master/src/extraction/FStar.Extraction.ML.Term.fst#L1882

@nikswamy
Copy link
Collaborator

nikswamy commented Dec 8, 2023

FWIW I tried also disabling --MLish on extraction everywhere and this also works.
I think I would like this better. And make --MLish meaningless for extraction.

Me too.

…em for dune

This eliminates the build race we observed, and seems like a good thing
to be able to look at both extractions of a file (in the overlap set of
fstarc and fstarlib).

`make package` still works.
@mtzguido
Copy link
Member Author

mtzguido commented Dec 8, 2023

Updated with:

  • No MLish special treatment on extraction
  • Do not pass the flag on --extract, since it does not do anything anyway
  • Separate the extraction outputs of the compiler and ulib, going into src/ocaml-output/fstarc/*.ml and src/ocaml-output/fstarlib/*.ml. These two are then copied in that order into ocaml/fstar-lib/generated when we are creating the snapshot. This eliminates the race, and I find it slightly cleaner. But we should work on improving the build instead.

Also FWIW I tried a build of the library without any compiler files, the result is very small, down to 1.7MB from 35M.

-r-xr-xr-x 1 guido guido 1.7M Dec  8 13:35 fstar_lib.cmxs

@mtzguido mtzguido merged commit 484b072 into FStarLang:master Dec 8, 2023
2 checks passed
@mtzguido mtzguido deleted the mlish_extraction branch December 8, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants