Skip to content

Conversation

@vszakats
Copy link
Member

@vszakats vszakats commented Dec 3, 2025

Before this patch runtests with TrackMemory-enabled curl run
memanalyze.pl twice after each executed test. Except for the common
case of AsynchDNS + non-c-ares (aka thread-resolver) builds, where
this step was skipped.

Patch #19786 removed this exception, which caused many CI jobs to run
memanalyze, resulting in a 10 (Linux) to 100% (2x, on Windows) increase
in test run times.

The intent before the patch above may have been to disable TrackMemory
for AsynchDNS builds, but what it really did was disabling tests 96,
558, 1330 that verified if TrackMemory is operating, displaying
a warning saying TrackMemory is disabled, but leaving memory tracking
enabled in curl (it's build-time option), which kept generating a
memdump, (but without runtests deleting it first). It also disabled
memanalyze.pl, to post-process memdump files.

The patch above removed the above logic, but with side-effects.

Fix by limiting memanalyze.pl post-processing to torture tests, which
seems to have been the original use-case for it, and the intent of
#19786 to enable it for all builds.

This also sped up existing jobs where memanalyze was accidentally run,
e.g. two c-ares Windows jobs, saving 4.5m per CI run.

Remaining questions: Why is memanalyze.pl so slow, esp. on Windows.
Could it be made faster? Could be run just once per test, not twice?
Could Windows torture tests made usable by making it fast? #19675

Bug: #19786 (comment)
Follow-up to fb7033d #19786

@vszakats vszakats marked this pull request as draft December 3, 2025 01:20
@github-actions github-actions bot added Windows Windows-specific CI Continuous Integration labels Dec 3, 2025
This reverts commit 73a1d1b.

Same perf, slightly worse maybe
@github-actions github-actions bot added the tests label Dec 3, 2025
@vszakats
Copy link
Member Author

vszakats commented Dec 3, 2025

Before fb7033d, AsyndDNS (= non-cares for most jobs) & TrackMemory jobs were tracking memory to the file memdump, but not running the memanalyze section in runtests.pl. After the patch, with memanalyze run in these jobs, they took a 2x perf hit, on Windows. It also took some hit on other OSes, but just 15% on macOS, and 10–30% on Linux.

In GHA/windows, there were two c-ares jobs that already had this overhead, before the patch above. E.g.
before: https://github.com/curl/curl/actions/runs/19740488711/job/56562906721
atfer: https://github.com/curl/curl/actions/runs/19824903607

windows arm64 MSVC timing details:

Before:

  Test suite total running time breakdown per task...
  
  Spent 0946.949 seconds starting and verifying test harness servers.
  Spent 0091.779 seconds reading definitions and doing test preparations.
  Spent 0927.179 seconds actually running test tools.
  Spent 0214.141 seconds awaiting server logs lock removal.
  Spent 0028.763 seconds verifying test results.
  Spent 2208.811 seconds doing all of the above.

After:

  Test suite total running time breakdown per task...
  
  Spent 0849.428 seconds starting and verifying test harness servers.
  Spent 0632.870 seconds reading definitions and doing test preparations.
  Spent 0923.245 seconds actually running test tools.
  Spent 0127.122 seconds awaiting server logs lock removal.
  Spent 0974.010 seconds verifying test results.
  Spent 3506.675 seconds doing all of the above.

macOS before: https://github.com/curl/curl/actions/runs/19740488688/job/56562906727
macOS after: https://github.com/curl/curl/actions/runs/19865498555/job/56927003248

Linux before: https://github.com/curl/curl/actions/runs/19740488712/job/56562906871 52s
Linux after: https://github.com/curl/curl/actions/runs/19865498547/job/56927003139 1m9

With the memanalyze pass disabled, Windows jobs are back to their previous perf:
https://github.com/curl/curl/actions/runs/19879931212/job/56975421948?pr=19819
(and the two c-ares jobs are running 2x fast - also explaining a long-time question why these were so much slower than others) (also running 96, 558, 1330 TrackMemory tests now.)

Questions:

  • why is memanalyze.pl so slow on Windows? (it's run twice, once normally and once with -v)
  • how to disable the memanalyze pass, at least on Windows. The goal of the referenced patch was to improve torture tests, but the jobs mentioned here are not torture tests. Is this adding any CI
  • or maybe there is a way to make it run fast on Windows?
  • the actual memory tracking was already done before this patch, so 96, 558 and 1330 could also be run, but because the feature flag was disabled they were skipped. The warning message said that memory tracking was disabled in these combination, but this didn't seem to be the case. What it disabled instead, was memanalyze, (and deleting a leftover memdump file, before running tests). I also don't understand how is this related to torture tests.

@vszakats vszakats changed the title GHA/windows: 2x perf drop [TEST] runtests: limit memanalyze.pl post-processing to torture tests Dec 3, 2025
@vszakats vszakats added performance and removed Windows Windows-specific labels Dec 3, 2025
@github-actions github-actions bot added the script label Dec 3, 2025
@vszakats vszakats marked this pull request as ready for review December 3, 2025 03:30
@bagder
Copy link
Member

bagder commented Dec 3, 2025

memanalyze scans the memdump file which is produced by all tests when memdebug is enabled. The script verifies that memory, socket and file use is okay. That is done with and without torture. On Windows, memdebug is even more important than Linux because in Windows land they don't have valgrind. When using valgrind, the memdebug setup is less important: but only memanalyze can verify total allocation use for example.

The memdebug tracking also helps us verify that individual test cases don't use more memory than allowed.

So memanalyze adds quite a lot of checking. I suppose the main reason it takes a hit on Windows is that Windows is slow in general for files and processes and it is started many times and it scans that memdump file for every test.

}

if($feature{"TrackMemory"}) {
if($torture) {
Copy link
Member

Choose a reason for hiding this comment

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

We want the file scanned for every yest. Torture and not torture.

@bagder
Copy link
Member

bagder commented Dec 3, 2025

The warning message said that memory tracking was disabled in these combination, but this didn't seem to be the case

It was. The feature capability TrackMemory was disabled in runtests for builds using a threaded resolver so test 96 etc wasn't run because the feature was missing.

@vszakats
Copy link
Member Author

vszakats commented Dec 4, 2025

Closing in favor of #19821

@vszakats vszakats closed this Dec 4, 2025
@vszakats vszakats deleted the slowin branch December 4, 2025 11:51
vszakats added a commit that referenced this pull request Dec 4, 2025
…t run)

Patch #19786 removed an exception, which caused many more CI jobs to run
`memanalyze.pl`. It resulted in a 10-30% (Linux), 15% (macOS), 100% (2x,
on Windows) slowdown of runtest steps. It also made some jobs exceed
their time limits and fail (seen with the Windows ARM64 job.)

Turns out the overhead was caused by calling `memanalyze.pl` as
an external process (twice per test), which in turn had to load a full
Perl stack from scratch each time.

Fix by converting memanalyze to a Perl modul, loaded as part of
`runtests.pl`, which eliminated the overhead completely.

It also sped up existing jobs where memanalyze was run for a long time,
e.g. two c-ares Windows jobs, saving 4.5m per CI run.

Supersedes #19819
Bug: #19786 (comment)
Follow-up to fb7033d #19786
Closes #19821
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants