Skip to content

Fix a memory leak by removing OPAMSTATS#6485

Merged
kit-ty-kate merged 1 commit intoocaml:masterfrom
hannesm:no-stats
Apr 29, 2025
Merged

Fix a memory leak by removing OPAMSTATS#6485
kit-ty-kate merged 1 commit intoocaml:masterfrom
hannesm:no-stats

Conversation

@hannesm
Copy link
Copy Markdown
Member

@hannesm hannesm commented Apr 24, 2025 •
edited by kit-ty-kate
Loading

especially in long-running applications that use opam as a library reading and/or writing lots of opam files, this lead to a huge amount of memory usage for no obvious gain.

Partially addresses #6484

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Apr 24, 2025

update: there was the environment variable OPAMSTATS, but I've not found anything on github that set it. of course, your backwards compatibility may differ -- but I find this specific form of statistics not very useful.

@kit-ty-kate
Copy link
Copy Markdown
Member

kit-ty-kate commented Apr 24, 2025

Personally, i'm fine with removing OPAMSTATS as it seems redundant with --debug-level=3/OPAMDEBUG, which already shows when files are read/written and which commands have been ran, with more useful information (the context where these things happen). LGTM

@kit-ty-kate kit-ty-kate added this to the 2.4.0~alpha2 milestone Apr 24, 2025
@kit-ty-kate kit-ty-kate changed the title remove statistics -- there was no way from the CLI to print them Fix a memory leak by removing OPAMSTATS Apr 29, 2025
@kit-ty-kate kit-ty-kate requested a review from rjbou April 29, 2025 05:26
rjbou
rjbou approved these changes Apr 29, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

I've added changelog API and mark OPAMSTATS env variable as removed instead of removing it completely.

@rjbou
Copy link
Copy Markdown
Collaborator

rjbou commented Apr 29, 2025

OPAMSTATS is indeed a legacy from initial opam implementation (0.9.0, cf e374647). As already noted, now debug information gives more information that those available via OPAMSTATS.
Discussed in dev meeting, it is indeed good to remove, especially given the memory usage and that now with patch library we will read much more files, this number will increase.

@rjbou
Copy link
Copy Markdown
Collaborator

rjbou commented Apr 29, 2025

Thank your for the PR @hannesm!

@rjbou
Copy link
Copy Markdown
Collaborator

rjbou commented Apr 29, 2025

For the records, the old output of stats was:

$ OPAMSTATS=1 opam install ago
The following actions will be performed:
=== install 1 package
  ∗ ago 0.4

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
⬇ retrieved ago.0.4  (cached)
∗ installed ago.0.4
Done.
42 files read:
  ~/.opam/myswitch/.opam-switch/build/ago.0.4/ago.install
  ~/.opam/myswitch/.opam-switch/reinstall
  ~/.opam/.last-env/env-c06a8218f9d2189637a24f578aedd01d-0
  ~/.opam/myswitch/.opam-switch/config/ocaml.config
  ~/.opam/myswitch/.opam-switch/overlay/opam-state/opam
  ~/.opam/myswitch/.opam-switch/overlay/opam-solver/opam
  ~/.opam/myswitch/.opam-switch/overlay/opam-repository/opam
  ~/.opam/myswitch/.opam-switch/overlay/opam-installer/opam
  ~/.opam/myswitch/.opam-switch/overlay/opam-format/opam
  ~/.opam/myswitch/.opam-switch/overlay/opam-devel/opam
  ~/.opam/myswitch/.opam-switch/overlay/opam-core/opam
  ~/.opam/myswitch/.opam-switch/overlay/opam-client/opam
  ~/.opam/myswitch/.opam-switch/overlay/ocp-index/opam
  ~/.opam/myswitch/.opam-switch/overlay/ocp-browser/opam
  ~/.opam/myswitch/.opam-switch/overlay/conf-witch/opam
  ~/.opam/myswitch/.opam-switch/overlay/cmdliner/opam
  ~/.opam/myswitch/.opam-switch/switch-state
  ~/.opam/myswitch/.opam-switch/switch-config
  [ switch-config files of existing switches ]
  ~/.opam/repo/repos-config
  ~/.opam/config
  ~/.opam/config
42 files write:
  ~/.opam/myswitch/.opam-switch/packages/ago.0.4/opam
  ~/.opam/myswitch/.opam-switch/environment
  ~/.opam/myswitch/.opam-switch/switch-state
  ~/.opam/myswitch/.opam-switch/install/ago.changes
  ~/.opam/myswitch/.opam-switch/install/ago.install
  ~/.opam/myswitch/.opam-switch/backup/state-20250428122113.export
6 external processes called:
  sh -c ocamlc -config | tr -d '\r' | grep '^ccomp_type: ' | sed -e 's/.*: //'
  sh -c ocamlc -config | tr -d '\r' | grep '^architecture: ' | sed -e 's/.*: //' -e 's/i386/i686/' -e 's/amd64/x86_64/'
  sh -c ocamlc -config | tr -d '\r' | grep '^os_type: ' | sed -e 's/.*: //' -e 's/Win32/msvc/' -e '/^msvc$/!s/.*/libc/'
  ocamlc -vnum
  lsb_release -s -r
  getprop ro.build.version.release

@rjbou rjbou requested a review from kit-ty-kate April 29, 2025 15:48
@kit-ty-kate kit-ty-kate merged commit a46d193 into ocaml:master Apr 29, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kit-ty-kate kit-ty-kate kit-ty-kate approved these changes

@rjbou rjbou rjbou approved these changes

Assignees

No one assigned

Labels

AREA: PERFORMANCE

Projects

None yet

Milestone

2.4.0~alpha2

Development

Successfully merging this pull request may close these issues.

3 participants

@hannesm @kit-ty-kate @rjbou