refs #4452 / refs #11705 - improved `--showtime=` behavior and testing #4876

firewave · 2023-03-08T18:48:28Z

This is a step onto leveraging the ThreadExecutor implementation for ProcessExecutor which is a follow-up to #4870. We need to have the proper test coverage and the existing implementations working as expected before we move to the shared code.

Fixes:

added --showtime= tests for all executor implementations
only print --showtime=summary once at the end
prevents --showtime= by multiple threads to be written at the same time - essentially breaking the output
reset the timer results before each test
deprecated top5 in favor of top5_file
fixed printing for all executors except ProcessExecutor

firewave · 2023-03-08T18:49:11Z

cli/cmdlineparser.cpp

    "    --rule-file=<file>   Use given rule file. For more information, see:\n"
    "                         http://sourceforge.net/projects/cppcheck/files/Articles/\n"
 #endif
+    // TODO: document --showtime


Will add after it is well defined after all my other comments/TODOs have been resolved.

lib/timer.cpp

firewave · 2023-03-08T18:53:40Z

test/testprocessexecutor.cpp

        check(2, 3, 3, oss.str());
    }

+    // TODO: check the output


Needs a thread-safe REDIRECT - will take some time to address this.

firewave · 2023-03-08T18:57:19Z

test/testprocessexecutor.cpp

+
+    // TODO: provide data which actually shows values above 0
+
+    // TODO: should this be logged only once like summary?


I think top5 makes sense in the summary and file context.

So I suggested we add top5_file and top5_summary and deprecate top5. We could keep top5 but it currently behaves like top5_file so that might be confusing.

firewave · 2023-03-08T18:58:43Z

@danmar I need some feedback on my comments to finish this up and move it out of draft. Thanks.

firewave · 2023-03-08T19:00:50Z

lib/timer.cpp

    }
 }

+// TODO: this does not include any file context when SHOWTIME_FILE thus rendering it useless - should we include the logging with the progress logging?


This would also be without context when using -q so tacking it with the progress won't help there and we need to add some kind of header.

firewave · 2023-04-13T13:08:29Z

Requires #4963 to be merged first.

firewave · 2023-05-04T16:15:01Z

The TSAN error with --showtime=file was always present we just never ran with that option in a TSAN context.

The locking for std::cout in timer.cpp needs to be replaced with using ErrorLogger::reportOut() which is properly synchronized when using multiple threads. I will address that in a future PR. I am still working on untangling all those existing error loggers.

firewave · 2023-09-26T20:49:52Z

Everything should be fixed except for the ProcessExecutor implementation. As noted above I will be tackling that in a different PR.

…other

…asserts for currently broken tests

… `top5_summary` / temporarily disabled collecting of timing information in daca

…d failed tests with TODO

…ple threads

…on and added TODO

…ication-result` clang-tidy warning

firewave · 2023-09-27T09:46:56Z

Some notes for the initial comment which should not be part of the commit message and might even be a bit outdated.

Notes:

the process implementation is currently broken and I have not applied fixes to it yet - it will be implicitly fixed when it leverages the thread implementation (PR coming soon)
--showtime= output might still be mixed up with the regular output from another thread. we also need to pipe the regular output through the SyncLogForwarder. this is only an issue with --showtime=. this will be fixed after the implementations are shared - possibly starting the verbose/stray std::cout usage cleanup - I have no idea how to address this yet.
--showtime= was never documented so any breakage should be allowed - besides it possibly never having worked properly

firewave · 2023-09-28T12:16:05Z

This is finally ready for review.

danmar

it looks good to me.. just a small spelling typo

danmar · 2023-10-05T16:51:42Z

cli/processexecutor.cpp

        }
    }

+    // TODO: wee need to get the timing information from the subprocess


spelling: wee => we

firewave commented Mar 8, 2023

View reviewed changes

lib/timer.cpp Show resolved Hide resolved

firewave commented Mar 8, 2023

View reviewed changes

firewave force-pushed the showtime branch 2 times, most recently from 840b39c to 9970e3a Compare March 10, 2023 08:30

firewave marked this pull request as draft March 20, 2023 19:02

firewave force-pushed the showtime branch from 9970e3a to c4321a8 Compare April 14, 2023 10:00

firewave force-pushed the showtime branch from c4321a8 to df453b7 Compare May 4, 2023 09:59

firewave marked this pull request as ready for review May 4, 2023 16:15

firewave marked this pull request as draft May 4, 2023 17:08

firewave force-pushed the showtime branch from f2fe86c to 5e524f7 Compare August 4, 2023 12:31

firewave force-pushed the showtime branch 4 times, most recently from da6dc5c to 6215566 Compare September 26, 2023 20:41

firewave force-pushed the showtime branch 2 times, most recently from 795c823 to 223a138 Compare September 27, 2023 08:06

firewave changed the title ~~refs #4452 - improved --showtime= behavior and testing~~ refs #4452 / refs #11705 - improved --showtime= behavior and testing Sep 27, 2023

firewave added 6 commits September 27, 2023 11:44

reset timer results before each test

791815e

TestThreadExecutor: run tests with quiet flag

f50a83b

TimerResults: avoid multiple threads writing their results over each …

b3f83a6

…other

TestThreadExecutor: added some showtime tests

a9b3826

actually only print the summary with --showtime=summary

61d6c2e

cmdlineparser.cpp: added TODO

f4704c8

firewave added 18 commits September 27, 2023 11:44

testthreadexecutor.cpp: added TODO

ae8c19d

Timer: removed TODOs

657a848

moved find_all_of() into cppcheck namespace and into helpers.h

0244d2e

TestThreadExecutor: inverted default quiet flag

c15117e

TestProcessExecutor: applied TestThreadExecutor changes / use TODO …

b3ab375

…asserts for currently broken tests

timer.cpp: added TODO

5a3abb3

cppcheck.cpp: suppress unusedFunction selfcheck warning

ff2a51a

deprecated --showtime=top5 and introduced the modes top5_file and…

8dbeda2

… `top5_summary` / temporarily disabled collecting of timing information in daca

renamed find_all_of() to count_all_of()

ef67e97

TestSingleExecutor: applied changes from other executor tests / marke…

dc02479

…d failed tests with TODO

fixed TSAN errors in Timer::stop() with --showtime=file and multi…

fcf6c89

…ple threads

Timer: use a lock for all std::cout uses

b2c5a56

print timing information in the proper places for SingleExecutor

176e679

ProcessExecutor: added (partial) missing printing of timing informati…

3a3e5d9

…on and added TODO

testprocessexecutor.cpp: fixed `bugprone-implicit-widening-of-multipl…

5a89705

…ication-result` clang-tidy warning

converted test_showtimetop5.sh into a Python test

d695765

added test coverage and documentation for --showtime=file-total

c2af315

Timer: added missing lock for SHOWTIME_FILE_TOTAL printing

2e8b8b9

firewave force-pushed the showtime branch from 223a138 to 2e8b8b9 Compare September 27, 2023 09:44

firewave marked this pull request as ready for review September 27, 2023 10:10

danmar approved these changes Oct 5, 2023

View reviewed changes

cli/processexecutor.cpp

}

}

// TODO: wee need to get the timing information from the subprocess

Copy link

Owner

danmar Oct 5, 2023

Choose a reason for hiding this comment

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

spelling: wee => we

firewave merged commit fc700b6 into danmar:main Oct 5, 2023

firewave deleted the showtime branch October 5, 2023 17:04


		// TODO: provide data which actually shows values above 0

		// TODO: should this be logged only once like summary?

refs #4452 / refs #11705 - improved --showtime= behavior and testing #4876

refs #4452 / refs #11705 - improved --showtime= behavior and testing #4876

Uh oh!

Conversation

firewave commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

firewave Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

firewave Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

firewave Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

danmar Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

firewave commented Mar 8, 2023

Uh oh!

firewave Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

firewave commented Apr 13, 2023

Uh oh!

firewave commented May 4, 2023

Uh oh!

firewave commented Sep 26, 2023

Uh oh!

firewave commented Sep 27, 2023

Uh oh!

firewave commented Sep 28, 2023

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

danmar Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

refs #4452 / refs #11705 - improved `--showtime=` behavior and testing #4876

refs #4452 / refs #11705 - improved `--showtime=` behavior and testing #4876

firewave commented Mar 8, 2023 •

edited

Loading