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.

Sign up for GitHub

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

Jump to bottom

Cleanup functor caches #1609

Merged
merged 7 commits into from May 23, 2023
Merged

Cleanup functor caches #1609

merged 7 commits into from May 23, 2023

Conversation

let-def
Copy link
Contributor

@let-def let-def commented May 17, 2023

This PR adds a mechanism to track the stamp of functor components cached in fcomp_cache and fcomp_subst_cache. They can they be removed from the cache when backtracking.

Its probably only necessary to track the components added to the persistent_env caches, this feature might be added later if this patch makes things too slow (or just worsen the problem :D).

@let-def
Create Stamped_hashtable structure
147f0c3
@let-def
Initial design was too simple
984ca92 
There are many tables scattered all over, we need
an indirection to gather all changes to a single place
@let-def
Implement functor cache backtracking using stamped env
00b85a0
@voodoos voodoos mentioned this pull request May 17, 2023
Closed
@voodoos
Copy link
Collaborator

voodoos commented May 17, 2023

This PR should fix #1529

@voodoos voodoos added this to Planned for backporting in Backport to 500 via automation May 17, 2023
voodoos
voodoos approved these changes May 22, 2023
View reviewed changes
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch @let-def !
I have a few questions, but the changes looks good to me.

Also I have confirmation that it fixes a usability issue in the Frama-C codebase: ocaml/ocaml-lsp#1032 (comment)

Do you think these changes could be accepted upstream ? (I can make the PR)
Also, could you add a changelog entry ?

src/ocaml/typing/env.ml Show resolved Hide resolved
src/ocaml/typing/env.ml Outdated Show resolved Hide resolved
src/utils/stamped_hashtable.mli Show resolved Hide resolved
@let-def
Copy link
Contributor Author

let-def commented May 23, 2023

Do you think these changes could be accepted upstream ?

I am not sure about that. The issue is very specific to Merlin.
It will never be problematic to the batch compiler, and an artificial example could show that the top-level suffers from the same problem but I am not sure it will be convincing enough to justify the added complexity.

The patch is designed to not be to intrusive, and so "easy" to port across different versions of Merlin and OCaml:

  • mostly of the change is code addition,
  • the only changes to existing code are the direct references to fcomp_cache and fcomp_subst_cache that needs to refer to Stamped_hashtable rather than Hashtbl.

Maybe a middle-ground could be to make the compiler defines aliases Stamped_hashtable = Hashtbl such that Merlin only needs to replace the Stamped_hashtable?

I am conjecturing a lot, we should talk to the compiler developers to get their actual thoughts :).

(I can make the PR) Also, could you add a changelog entry ?

Yep.

@voodoos voodoos added this to Todo in Backport to 3.4 via automation May 23, 2023
@voodoos voodoos merged commit f259e5c into master May 23, 2023
4 of 5 checks passed
Backport to 500 automation moved this from Planned for backporting to Ready for backporting May 23, 2023
@voodoos voodoos moved this from Ready for backporting to 4.9-500 in Backport to 500 May 26, 2023
voodoos added a commit to voodoos/merlin that referenced this pull request May 26, 2023
@voodoos
[B] Cleanup functor caches (ocaml#1609)
8868e0b 
 from ocaml/cleanup-caches
voodoos added a commit to voodoos/opam-repository that referenced this pull request May 26, 2023
@voodoos
[new release] merlin, merlin-lib and dot-merlin-reader (4.9-414)
b9155d2 
CHANGES:

Fri May 26 15:23:42 CEST 2023

  + merlin binary
    - Allow monadic IO in dot protocol (ocaml/merlin#1581)
    - Add a `scope` option to the `occurrences` command in preparation for
      the upcoming `project-wide-occurrences` feature (ocaml/merlin#1596)
    - Construct bool-typed holes as `false` instead of `true` in the
      `construct` command, for consistency (ocaml/merlin#1599).
    - Add a hook to configure system command for spawning ppxes when Merlin is
      used as a library. (ocaml/merlin#1585)
    - Implement an all-or-nothing cache for the PPX phase (ocaml/merlin#1584)
    - Cleanup functors caches when backtracking, to avoid memory leaks
      (ocaml/merlin#1609, fixes ocaml/merlin#1529 and ocaml-lsp#1032)
    - Fix `construct` results ordering for sum types sand poly variants (ocaml/merlin#1603)
    - Fix object method completion not working (ocaml/merlin#1606, fixes ocaml/merlin#1575)
    - Improve context detection for package types (ocaml/merlin#1608, fixes ocaml/merlin#1607)
    - Fix incorrect locations for string literals (ocaml/merlin#1574)
    - Fixed an issue that caused `errors` to erroneously alert about missing
      `cmi` files (ocaml/merlin#1577)
    - Prevent destruct from crashing on closed variant types (ocaml/merlin#1602,
      fixes ocaml/merlin#1601)
    - Improve longident parsing (ocaml/merlin#1612, fixes ocaml/merlin#945)
  + editor modes
    - emacs: call the user's configured completion UI in
      `merlin-construct` (ocaml/merlin#1598)
  + test suite
    - Add missing dependency to a test using ppxlib (ocaml/merlin#1583)
    - Add tests for the new PPX phase cache (ocaml/merlin#1584)
    - Add and update tests for `construct` ordering (ocaml/merlin#1603)
voodoos added a commit to voodoos/opam-repository that referenced this pull request May 26, 2023
@voodoos
[new release] merlin, merlin-lib and dot-merlin-reader (4.9-500)
4dbf488 
CHANGES:

Fri May 26 15:23:42 CEST 2023

  + merlin binary
    - Allow monadic IO in dot protocol (ocaml/merlin#1581)
    - Add a `scope` option to the `occurrences` command in preparation for
      the upcoming `project-wide-occurrences` feature (ocaml/merlin#1596)
    - Construct bool-typed holes as `false` instead of `true` in the
      `construct` command, for consistency (ocaml/merlin#1599).
    - Add a hook to configure system command for spawning ppxes when Merlin is
      used as a library. (ocaml/merlin#1585)
    - Implement an all-or-nothing cache for the PPX phase (ocaml/merlin#1584)
    - Cleanup functors caches when backtracking, to avoid memory leaks
      (ocaml/merlin#1609, fixes ocaml/merlin#1529 and ocaml-lsp#1032)
    - Fix `construct` results ordering for sum types sand poly variants (ocaml/merlin#1603)
    - Fix object method completion not working (ocaml/merlin#1606, fixes ocaml/merlin#1575)
    - Improve context detection for package types (ocaml/merlin#1608, fixes ocaml/merlin#1607)
    - Fix incorrect locations for string literals (ocaml/merlin#1574)
    - Fixed an issue that caused `errors` to erroneously alert about missing
      `cmi` files (ocaml/merlin#1577)
    - Prevent destruct from crashing on closed variant types (ocaml/merlin#1602,
      fixes ocaml/merlin#1601)
    - Improve longident parsing (ocaml/merlin#1612, fixes ocaml/merlin#945)
  + editor modes
    - emacs: call the user's configured completion UI in
      `merlin-construct` (ocaml/merlin#1598)
  + test suite
    - Add missing dependency to a test using ppxlib (ocaml/merlin#1583)
    - Add tests for the new PPX phase cache (ocaml/merlin#1584)
    - Add and update tests for `construct` ordering (ocaml/merlin#1603)
voodoos added a commit to voodoos/opam-repository that referenced this pull request May 26, 2023
@voodoos
[new release] merlin, merlin-lib and dot-merlin-reader (4.9-414/500)
bc1219b 
CHANGES:

Fri May 26 15:23:42 CEST 2023

  + merlin binary
    - Allow monadic IO in dot protocol (ocaml/merlin#1581)
    - Add a `scope` option to the `occurrences` command in preparation for
      the upcoming `project-wide-occurrences` feature (ocaml/merlin#1596)
    - Construct bool-typed holes as `false` instead of `true` in the
      `construct` command, for consistency (ocaml/merlin#1599).
    - Add a hook to configure system command for spawning ppxes when Merlin is
      used as a library. (ocaml/merlin#1585)
    - Implement an all-or-nothing cache for the PPX phase (ocaml/merlin#1584)
    - Cleanup functors caches when backtracking, to avoid memory leaks
      (ocaml/merlin#1609, fixes ocaml/merlin#1529 and ocaml-lsp#1032)
    - Fix `construct` results ordering for sum types sand poly variants (ocaml/merlin#1603)
    - Fix object method completion not working (ocaml/merlin#1606, fixes ocaml/merlin#1575)
    - Improve context detection for package types (ocaml/merlin#1608, fixes ocaml/merlin#1607)
    - Fix incorrect locations for string literals (ocaml/merlin#1574)
    - Fixed an issue that caused `errors` to erroneously alert about missing
      `cmi` files (ocaml/merlin#1577)
    - Prevent destruct from crashing on closed variant types (ocaml/merlin#1602,
      fixes ocaml/merlin#1601)
    - Improve longident parsing (ocaml/merlin#1612, fixes ocaml/merlin#945)
  + editor modes
    - emacs: call the user's configured completion UI in
      `merlin-construct` (ocaml/merlin#1598)
  + test suite
    - Add missing dependency to a test using ppxlib (ocaml/merlin#1583)
    - Add tests for the new PPX phase cache (ocaml/merlin#1584)
    - Add and update tests for `construct` ordering (ocaml/merlin#1603)

[new release] merlin, merlin-lib and dot-merlin-reader (4.9-414)

CHANGES:

Fri May 26 15:23:42 CEST 2023

  + merlin binary
    - Allow monadic IO in dot protocol (ocaml/merlin#1581)
    - Add a `scope` option to the `occurrences` command in preparation for
      the upcoming `project-wide-occurrences` feature (ocaml/merlin#1596)
    - Construct bool-typed holes as `false` instead of `true` in the
      `construct` command, for consistency (ocaml/merlin#1599).
    - Add a hook to configure system command for spawning ppxes when Merlin is
      used as a library. (ocaml/merlin#1585)
    - Implement an all-or-nothing cache for the PPX phase (ocaml/merlin#1584)
    - Cleanup functors caches when backtracking, to avoid memory leaks
      (ocaml/merlin#1609, fixes ocaml/merlin#1529 and ocaml-lsp#1032)
    - Fix `construct` results ordering for sum types sand poly variants (ocaml/merlin#1603)
    - Fix object method completion not working (ocaml/merlin#1606, fixes ocaml/merlin#1575)
    - Improve context detection for package types (ocaml/merlin#1608, fixes ocaml/merlin#1607)
    - Fix incorrect locations for string literals (ocaml/merlin#1574)
    - Fixed an issue that caused `errors` to erroneously alert about missing
      `cmi` files (ocaml/merlin#1577)
    - Prevent destruct from crashing on closed variant types (ocaml/merlin#1602,
      fixes ocaml/merlin#1601)
    - Improve longident parsing (ocaml/merlin#1612, fixes ocaml/merlin#945)
  + editor modes
    - emacs: call the user's configured completion UI in
      `merlin-construct` (ocaml/merlin#1598)
  + test suite
    - Add missing dependency to a test using ppxlib (ocaml/merlin#1583)
    - Add tests for the new PPX phase cache (ocaml/merlin#1584)
    - Add and update tests for `construct` ordering (ocaml/merlin#1603)
voodoos added a commit to voodoos/opam-repository that referenced this pull request May 31, 2023
@voodoos
[new release] merlin, merlin-lib and dot-merlin-reader (4.9-501preview)
80012cd 
CHANGES:

unreleased

  + merlin binary
    - Preview support for OCaml 5.1-alpha1. Short path is temporary disabled and
      inline records might not behave as expected.
    - Allow monadic IO in dot protocol (ocaml/merlin#1581)
    - Add a `scope` option to the `occurrences` command in preparation for
      the upcoming `project-wide-occurrences` feature (ocaml/merlin#1596)
    - Construct bool-typed holes as `false` instead of `true` in the
      `construct` command, for consistency (ocaml/merlin#1599).
    - Add a hook to configure system command for spawning ppxes when Merlin is
      used as a library. (ocaml/merlin#1585)
    - Implement an all-or-nothing cache for the PPX phase (ocaml/merlin#1584)
    - Cleanup functors caches when backtracking, to avoid memory leaks
      (ocaml/merlin#1609, fixes ocaml/merlin#1529 and ocaml-lsp#1032)
    - Fix `construct` results ordering for sum types sand poly variants (ocaml/merlin#1603)
    - Fix object method completion not working (ocaml/merlin#1606, fixes ocaml/merlin#1575)
    - Improve context detection for package types (ocaml/merlin#1608, fixes ocaml/merlin#1607)
    - Fix incorrect locations for string literals (ocaml/merlin#1574)
    - Fixed an issue that caused `errors` to erroneously alert about missing
      `cmi` files (ocaml/merlin#1577)
    - Prevent destruct from crashing on closed variant types (ocaml/merlin#1602,
      fixes ocaml/merlin#1601)
    - Improve longident parsing (ocaml/merlin#1612, fixes ocaml/merlin#945)
  + editor modes
    - emacs: call the user's configured completion UI in
      `merlin-construct` (ocaml/merlin#1598)
  + test suite
    - Add missing dependency to a test using ppxlib (ocaml/merlin#1583)
    - Add tests for the new PPX phase cache (ocaml/merlin#1584)
    - Add and update tests for `construct` ordering (ocaml/merlin#1603)
@voodoos voodoos mentioned this pull request May 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voodoos voodoos voodoos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Backport to 3.4
Todo
Backport to 500
4.9-500
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@let-def @voodoos