Option to remove fflush in mhvtl code.

Calls to the method fflush in mhvtl code makes slower run the software in our Virtual Machines. Set an option to disable fflush.

added Continuous Integration label

assigned to @jcamarer

added 1 deleted label

changed milestone to %CI update push

marked this issue as related to #177 (closed)

assigned to @mdavis

The lines with fflush are the following ones (1.7-0_release):

tcopy/tcopy.c:177:                      (void) fflush(stdout);
usr/vtllib.c:660:               fflush(NULL);
usr/vtllib.c:723:       fflush(fifo_fd);
usr/logging.h:37:               fflush(NULL);                                   \
usr/logging.h:48:               fflush(NULL);                                   \
usr/smc.c:458:  fflush(NULL);
usr/vtllibrary.c:1826:          fflush(NULL);
usr/vtllibrary.c:1960:                  fflush(NULL);   /* So I can pipe debug o/p thru tee */
usr/vtltape.c:2443:             fflush(NULL);
usr/vtltape.c:2556:                     fflush(NULL)

Thanks. However, some of those (logging.h) I guess are just for flushing the logfile to disk, not for flushing data to tape.

Do we know which of these have an impact on our CI performance?

To get a baseline, could you measure how long it takes to run all the CI tests with the stock mhVTL? (Run the tests 5+ times to average out statistical noise from the GitLab infrastructure).

Then we can compare with how long it takes with all the flush statements removed and how much we actually gain by using this hacked mhVTL version.

added workflowIn Progress label and removed 1 deleted label

added mhVTL label

I ran tests/test_client.sh on a new runner without the fflush: ./run_systemtest.sh part took 22 minutes on commit e4585d8d.

In main the same test ran the same section in 20 minutes.

Overhead does not looks like a big deal but this is not measuring something really useful:

• new runners are faster (better hardware underneath the VMs)
• this is only 10k files

We need a beefier test comparing apples with apples: at least 200k files archived/retrieved.

This measurement just says: we can replace CI runners with the new alma9 setup with no big impact in runtime.

I contacted Mark (Harvey) who reports:

Most of the fflush() are there for debugging purposes (flush stdout). Currently doing a quick audit and will hide them behind a if (debug). The only fflush required is before a seek or media eject.

I suspect the one that is required is the one we care about, but let's see what he comes back with.

Mark has committed the changes to mhvtl:

commit 01e1cacd73e78caed3b82d935de0f6bb7746cf5d (HEAD -> master, origin/master, origin/HEAD)
Author: Mark Harvey <markh794@gmail.com>
Date:   Fri Jun 23 10:51:46 2023 +1000

    only call fflush() while in debug mode.

We should build this version and test it in CI. If everything looks good, we can ask him to tag a new release. Then we can eliminate our homebrew version.

added Needs Discussion label

assigned to @jleduc

removed Needs Discussion label

unassigned @jcamarer

added prioritymedium label

added prioritylow label and removed prioritymedium label

This has already been done. Mark changed the code so taht the fflush only runs when the debug flag is set. This got included in tag 1.7.1.

But before this tag we were also running mhVTL without the fflush as the rpms we were generating had this lines remove manually: https://gitlab.cern.ch/cta/cta-dependencies/-/blob/b0108ef8aac9724a16c0e34c71c2b4591a1dc45e/mhvtl/mhvtl-utils.spec#L102 This link is in master/main at the date of writing this comment.

I am in the process of having a stable way of creating RPMs from latest or from a tag. Closing this ticket as it no longer makes senese.

closed