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

Add a profiling tool in GnuCOBOL #110

Closed

Conversation

emilienlemaire
Copy link

This PR would add a simple profiling tool in GnuCOBOL which computes the time spent in each section and each paragraph of the COBOL program.

Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this. Apart from formatting as centered/number, I fail to see a use in the xlsx - please note which benefits you see in that.

The created .csv will work with different table calculation programs (and on the command line and also as a log that's easy to query and/or use in log collection), so this should be definitely the default. I'm actually not sure if we should keep xlsx at all (note: it would be possible to provide a macro file for different programs that load a created perf.csv and nicely format it on the fly; and if we move it out, then you'd still be able to provide a file to be LD_PRELOADED [possibly COB_PRE_LOAD works as well?] for writing XSLT files).

From the data part: shouldn't CALLs be profiled separately, allowing to see (also for programs not compiled with profiling / non COBOL functions) how long the time is spent in there? This would also allow an output "self/inclusive".

libcob/cobprof.h Outdated Show resolved Hide resolved
libcob/cobprof.h Outdated Show resolved Hide resolved
libcob/common.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
tests/testsuite.src/used_binaries.at Outdated Show resolved Hide resolved
tests/testsuite.src/used_binaries.at Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2023

Codecov Report

Attention: Patch coverage is 86.81102% with 67 lines in your changes are missing coverage. Please review.

Project coverage is 66.06%. Comparing base (89c45a3) to head (67c02ad).

Files Patch % Lines
libcob/profiling.c 87.25% 21 Missing and 11 partials ⚠️
libcob/common.c 64.77% 22 Missing and 9 partials ⚠️
cobc/codegen.c 98.52% 0 Missing and 2 partials ⚠️
cobc/parser.y 90.90% 1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@                  Coverage Diff                  @@
##           gcos4gnucobol-3.x     #110      +/-   ##
=====================================================
+ Coverage              65.87%   66.06%   +0.19%     
=====================================================
  Files                     32       33       +1     
  Lines                  59483    59947     +464     
  Branches               15709    15778      +69     
=====================================================
+ Hits                   39182    39603     +421     
- Misses                 14282    14307      +25     
- Partials                6019     6037      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@emilienlemaire
Copy link
Author

Thank you for the review.
To answer your question, the xlsx support was implemented as I saw this was the standard output format of most proprietary COBOL profiling tools, as such I thought it would be a nice feature to add to this implementation.
I have few things planned for this PR, mainly adding a flag to select between csv and xlsx if the latter is available, and the CALL profiling also, I just wanted some input on your side, hence the publication.

@GitMensch
Copy link
Collaborator

We commonly don't agree with proprietary COBOL compilers on many points, and as there is no real feature in that and people can open the csv in Excel - let's drop that format and the added checks, code and docs for the dependencies.
If we see that there would be much benefit in it, then we can still add more formats later (but in this case that likely would be json [with the current JSON implementation as backend] and/or xml [with libxml2 as backend]).

Note that csv and json/xml are also much more preferable if you want to use the data in your test or even CI environment.

@lefessan
Copy link
Member

people can open the csv in Excel - let's drop that format and the added checks, code and docs for the dependencies.

The choice is not between csv and xls, but to have both of them for different users: unix hackers will choose csv and use additional tools or scripts to extract data, but windows devs might prefer xls, because it is easier to view (csv always ask about the format to use to parse the file) and easier to graphically customize (adding colors to highlight the most important values, etc.). I think it would be better to have xls now, and then to drop it when powerful tools are available to post process csv files, rather than having only csv output now and never seeing such tools appear, or hard to find in the contribs...

@GitMensch
Copy link
Collaborator

Excel is also able to open Open Document Spreadsheeds (ods files), I'd be ok with adding this, but again - to reduce the complexity of the changes I suggest to split this into a second PR, one "just the feature", then another "adding a format which can be chosen by the user".
Note: a simple ODS file is just minimal plain XML (can be created as text) named content.xml, put into an archive named ods. That doesn't even need any library (using zlib or similar to create the archive from memory instead of creating the xml text file up front would be useful (zlib I guess), but not mandatory (if not available a system call gzip would be executed).

@emilienlemaire
Copy link
Author

Ok I will make this PR "feature only" with csv file generation only, and make another one later with ODS file format.
The deletion of xlsx will come with the next commit, along with the other requested features / fixes

Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

We should have an extra variable that needs to be set to turn profiling on - like COB_SET_TRACE (this way you can compile it in but can selectively enable it, both for a specific process or even - with code adjustments of SET ENVIRONMENT... for only part of run.

In this case the "counting" functions would check this flag.
This would also be different from tools like gprof where we can adjust the profiling only per source unit.

libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we do have a single file in the runtime for this feature - please use it to give an outline how the profiling works (an overview and which function is called where/why, maybe similar to what strings.c has before cob_string_init; each function should get a doc comment which may also provide more detailed information).

Copy link
Collaborator

Choose a reason for hiding this comment

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

pending TODO

libcob/cobprof.c Outdated Show resolved Hide resolved
tests/testsuite.src/used_binaries.at Outdated Show resolved Hide resolved
libcob/common.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
@GitMensch
Copy link
Collaborator

And, as a "general" thought: instead of resolving the "textual" data in libcob and storing it there, this part should likely be postponed until there's an actual write to the profiling file (when all the section/paragraph/location info can be requested/read). Switching to this approach should both save memory during collection and reduce the profiling overhead.

@GitMensch
Copy link
Collaborator

@lefessan If I understood this right, you're likely to take on this addition next, right?

@lefessan
Copy link
Member

Yes, I have started fixing some of the issues you raised, but I still need some more time to get a better global picture of the patch before doing further changes.

@GitMensch GitMensch marked this pull request as draft October 20, 2023 04:41
@GitMensch
Copy link
Collaborator

Any chance on getting this done in the next two weeks?
If not then we'll postpone it.

@lefessan
Copy link
Member

lefessan commented Nov 2, 2023

Ok, I will do it or see if somebody else in the team can !

@lefessan lefessan force-pushed the perf-line branch 9 times, most recently from f09c48d to 44c6e92 Compare November 14, 2023 14:18
@lefessan lefessan marked this pull request as ready for review November 14, 2023 14:20
@lefessan lefessan force-pushed the perf-line branch 2 times, most recently from 4b2ac13 to 42459b3 Compare November 14, 2023 14:42
doc/gnucobol.texi Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/profiling.c Outdated Show resolved Hide resolved
libcob/profiling.c Outdated Show resolved Hide resolved
libcob/profiling.c Outdated Show resolved Hide resolved
libcob/profiling.c Outdated Show resolved Hide resolved
libcob/profiling.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/common.c Outdated Show resolved Hide resolved
libcob/cobprof.c Outdated Show resolved Hide resolved
libcob/profiling.c Outdated Show resolved Hide resolved
libcob/profiling.c Outdated Show resolved Hide resolved
libcob/profiling.c Outdated Show resolved Hide resolved
@ddeclerck
Copy link
Contributor

Hi @GitMensch ,
Would this be okay to be merged now ?

Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

I consider this my final review of this PR - the overall state is "very good" already.
Feel free to directly commit this after taking the review comments into account.

Thank you all for your patience!

cobc/ChangeLog Show resolved Hide resolved
cobc/codegen.c Outdated Show resolved Hide resolved
cobc/codegen.c Outdated Show resolved Hide resolved
cobc/codegen.c Outdated Show resolved Hide resolved
libcob/common.c Outdated Show resolved Hide resolved
libcob/ChangeLog Show resolved Hide resolved
libcob/profiling.c Outdated Show resolved Hide resolved
libcob/profiling.c Outdated Show resolved Hide resolved
config/runtime.cfg Outdated Show resolved Hide resolved
config/runtime.cfg Show resolved Hide resolved
@ddeclerck ddeclerck force-pushed the perf-line branch 4 times, most recently from 89a3a6b to fe48125 Compare April 8, 2024 14:12
Initial work by Emilien Lemaire <emilien.lemaire@ocamlpro.com>

Additional features:
* Do not check for enter/exit section: instead, sum time from paragraphs
* Support for modules, CALL and ENTRY points
* Support for recursive calls
* Allocate virtual stack on demand instead of statically
* Correct handling of EXIT PARAGRAPH code with 'goto'
* Prevent CANCEL from dlclose a module during profiling
* Customize CSV result file with COB_PROF_FORMAT
* Customize CSV filename using $b/$f/$d/$t
* Add some tests for RECURSIVE on PROGRAM-ID
* fix compile under WIN32 and systems without CLOCK_MONOTONIC / CLOCK_MONOTONIC_RAW
* fix use of defined but unavailable CLOCK_MONOTONIC_RAW
* minor refactoring
Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

Thank you all for the work on this. The PR is good to be merged uptream now (with the two commented changes either in or get in later).

libcob/common.c Outdated Show resolved Hide resolved
@ddeclerck
Copy link
Contributor

I think that's it ? If you're okay with the way I handled the change about relative-to-absolute path conversion.

@GitMensch
Copy link
Collaborator

That's fine, just add a changelog entry for the function extraction when doing the check-in "upstream".

For the test - I'd tend to have two AC_TEST and think 2>/dev/null is not needed for the first (is it?) and for the ls instead of >/dev/null we should use "ignore" for stdout.

@ddeclerck
Copy link
Contributor

For the test - I'd tend to have two AC_TEST and think 2>/dev/null is not needed for the first (is it?)

Well, the runtime outputs File XXX generated to stderr when generating profiling infos. I can use ignore for that too instead of redierction.

@GitMensch
Copy link
Collaborator

closed as it is merged upstream (it would be good to update the git mirror of course, but that's a different thing.

@GitMensch GitMensch closed this Apr 20, 2024
@GitMensch GitMensch added reopened (merged) already merged, but reopened and removed Ready for SVN labels May 2, 2024
@GitMensch GitMensch reopened this May 2, 2024
@GitMensch
Copy link
Collaborator

GitMensch commented May 2, 2024

checking common.h I've recognized that there is one change that should be applied here: the profiling types should be "anonymous" in common.h (then doesn't need more than typedef struct cob_prof_module cob_prof_module;) and only be defined in profiling.c because of encapsulation - we don't want them to be modified outside of profiling.c - see comment in #137 (comment)

@ddeclerck Sorry for not catching this beforehand - can you please adjust the code with a follow-up commit?

@GitMensch
Copy link
Collaborator

GitMensch commented May 2, 2024

Sadly #140 was not pulled in before - the VC CI builds are broken since 23 days -as there is a new file and an adjustment to libcob/Makefile.am - but no changes to build_windows//libcob - can you please fix these (similar to r5112 - 8bc9fed) and change to use the anonymous type?

Oh - and while working on this - GCC says:

profiling.c: In function 'cob_prof_print_line':
profiling.c:449:41: warning: 'line' may be used uninitialized in this function [-Wmaybe-uninitialized]
  449 |                                         fprintf (file, "%d", line);
      |                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~

so please check the paths (most times but not always gcc is right) and only if it is a false-positive initialize the variable to zero.

@GitMensch
Copy link
Collaborator

closed per follow-up PR "nearly checked in"

@GitMensch GitMensch closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged in SVN reopened (merged) already merged, but reopened
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants