Re: [Tarantool-patches] [PATCH v1 luajit 41/41] ci: introduce the performance workflow

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review.
Please, consider my answers below.

On 18.11.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! See my comments.
> 
> Sergey
> 
> On 10/24/25 14:00, Sergey Kaplun wrote:
> > This patch adds the workflow to run benchmarks from various suites,
> > aggregate their results, and send statistics to the InfluxDB to be
> > processed later.
> >
> > The workflow contains a matrix to measure GC64 and non-GC64 modes with
> > enabled/disabled JIT for x64 architecture.
> > ---

<snipped>

> > +      run: |
> > +        apt -y update
> > +        apt install -y luarocks curl
> > +      shell: bash
> > +    - name: Install Lua modules
> > +      run: luarocks install lua-cjson
> > +      shell: bash
> > +    - name: Run script to setup Linux environment
> > +      run: sh ./perf/helpers/setup_env.sh
> > +      shell: bash
> bash or shell is used in the last step? (shebang in setup_env.sh is 
> /bin/sh)

It uses the `sh` command anyway, but I prefer to leave this line as is
for the consistency.

> > diff --git a/.github/workflows/performance.yml b/.github/workflows/performance.yml
> > new file mode 100644
> > index 00000000..bfb6be97
> > --- /dev/null
> > +++ b/.github/workflows/performance.yml

<snipped>

> > +concurrency:
> > +  # An update of a developer branch cancels the previously
> > +  # scheduled workflow run for this branch. However, the default
> > +  # branch, and long-term branch (tarantool/release/2.11,
> > +  # tarantool/release/2.10, etc) workflow runs are never canceled.
> > +  #
> it is not relevant, right?

Why not? Unless we send the results to the InfluxDB, the job isn't
finished and may be aborted that way for the default branch if we
need to push dummy commits.

> > +  # We use a trick here: define the concurrency group as 'workflow
> > +  # run ID' + # 'workflow run attempt' because it is a unique
> > +  # combination for any run. So it effectively discards grouping.
> > +  #
> > +  # XXX: we cannot use `github.sha` as a unique identifier because
> > +  # pushing a tag may cancel a run that works on a branch push
> > +  # event.
> > +  group: ${{ startsWith(github.ref, 'refs/heads/tarantool/')
> > +    && format('{0}-{1}', github.run_id, github.run_attempt)
> > +    || format('{0}-{1}', github.workflow, github.ref) }}
> > +  cancel-in-progress: true
> > +

<snipped>

> > +      - name: configure
> > +        # The taskset alone will pin all the process threads
> > +        # into a single (random) isolated CPU, see
> > +        #https://bugzilla.kernel.org/show_bug.cgi?id=116701.
> > +        # The workaround is using realtime scheduler for the
> > +        # isolated task using chrt, e. g.:
> > +        # sudo taskset 0xef chrt 50.
> > +        # But this makes the process use non-standard, real-time
> > +        # round-robin scheduling mechanism.
> > +        run: >
> > +          cmake -S . -B ${{ env.BUILDDIR }}
> > +          -DCMAKE_BUILD_TYPE=RelWithDebInfo
> 
> RelWithDebInfo is -O2 (moderate optimization), Release is -O3 (high 
> optimization).
> 
> Do we really need  RelWithDebInfo? I think it deserves a comment.

It is the default build for the Tarantool as well. We have no Release
builds, IINM. Also, mostly we executed the hot code either in the VM or
on trace, where these flags are irrelevant (excluding table lookups).

> 
> > +          -DLUAJIT_ENABLE_PERF=ON
> > +          -DLUAJIT_BENCH_INIT="taskset 0xfe chrt 50"
> > +          -DLUAJIT_DISABLE_JIT=${{ matrix.JOFF }}
> > +          -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}

<snipped>

> > +      - name: perf
> this name is visible in Web UI. I would make it more descriptive (execute
> performance benchmarks?)

I prefer to leave it as is to be consistent with naming in tests. All
description is given in the workflow name anyway.

<snipped>

Also, fixed the workflow name:

===================================================================
diff --git a/.github/workflows/performance.yml b/.github/workflows/performance.yml
index bfb6be97..fe22eb4b 100644
--- a/.github/workflows/performance.yml
+++ b/.github/workflows/performance.yml
@@ -56,7 +56,7 @@ jobs:
     name: >
       LuaJIT
       GC64:${{ matrix.GC64 }}
-      JOFF:${{ matrix.GC64 }}
+      JOFF:${{ matrix.JOFF }}
     steps:
       - uses: actions/checkout@v4
         with:
===================================================================

-- 
Best regards,
Sergey Kaplun