benchmark-action / github-action-benchmark

GitHub Action for continuous benchmarking to keep performance
https://benchmark-action.github.io/github-action-benchmark/dev/bench/
MIT License
1.02k stars 152 forks source link

Golang Benchmarks don't take Package Name into consideration #264

Open gaby opened 2 months ago

gaby commented 2 months ago

When running benchmarks in a repo with a lot of packages, if two benchmarks have the same name this github-action doesn't take into account the package name cause the comparison to fail randomly.

This can be seen on this run: https://github.com/gofiber/fiber/actions/runs/10873073504

Where BenchmarkAppendMsgitem shows up multiple times with different values even though they are in different packages.

ktrz commented 2 months ago

Hey @gaby Thanks for reporting this! I wonder how we could deal with this. I think it will create a similar issue of backward compatibility as we faced with PR #177

We could append the package name to the name of the benchmark so it would be eg

BenchmarkAppendMsgitem (github.com/gofiber/fiber/v3/middleware/cache)

This should create a unique name to compare As a remedy to not break existing metrics we could treat benchmarks with only one package as special cases and not append the package name in those cases. Or add both cases: old (without suffix) and new (with suffix) so that it can be then dropped in the next release. WDYT?

gaby commented 2 months ago

@ktrz Adding both cases is probably the way to go now. Other option I can think is having a config option to specify if you want benchmarks with suffix or not.

gaby commented 1 month ago

@ktrz Any progress on this issue? Thanks!

ReneWerner87 commented 1 week ago

@ktrz what is the status? the package name after the name of the benchmark would be good and would solve the problem

ReneWerner87 commented 6 days ago

problem output

PASS
ok      github.com/gofiber/fiber/v3/log 0.173s
PASS
ok      github.com/gofiber/fiber/v3/middleware/adaptor  0.184s
PASS
ok      github.com/gofiber/fiber/v3/middleware/basicauth    0.173s
goos: darwin
goarch: arm64
pkg: github.com/gofiber/fiber/v3/middleware/cache
BenchmarkAppendMsgitem
BenchmarkAppendMsgitem-12       63634455            19.01 ns/op 3103.57 MB/s           0 B/op          0 allocs/op
BenchmarkAppendMsgitem-12       66411781            18.42 ns/op 3202.97 MB/s           0 B/op          0 allocs/op
PASS
ok      github.com/gofiber/fiber/v3/middleware/cache    2.649s
PASS
ok      github.com/gofiber/fiber/v3/middleware/compress 0.219s
PASS
ok      github.com/gofiber/fiber/v3/middleware/cors 0.188s
goos: darwin
goarch: arm64
pkg: github.com/gofiber/fiber/v3/middleware/csrf
BenchmarkAppendMsgitem
BenchmarkAppendMsgitem-12       1000000000           0.2926 ns/op   3417.23 MB/s           0 B/op          0 allocs/op
BenchmarkAppendMsgitem-12       1000000000           0.2883 ns/op   3468.54 MB/s           0 B/op          0 allocs/op
PASS
ok      github.com/gofiber/fiber/v3/middleware/csrf 0.842s
PASS

current code

https://github.com/benchmark-action/github-action-benchmark/blob/6bae118c112083251560ad8b3a1ff2e43aa23351/src/extract.ts#L342-L392

possible improvement

function extractGoResult(output: string): BenchmarkResult[] {
    const lines = output.split(/\r?\n/g);
    const results: BenchmarkResult[] = [];
    let currentPackage = '';

    // Regular expression to extract the package path
    const packageRegex = /^pkg:\s+(.+)$/;

    // Regular expression to extract benchmark information
    const benchmarkRegex =
        /^(?<name>Benchmark\w+[\w()$%^&*-=|,[\]{}"#]*?)(?<procs>-\d+)?\s+(?<times>\d+)\s+(?<remainder>.+)$/;

    for (const line of lines) {
        // Check if the line contains the package path
        const packageMatch = line.match(packageRegex);
        if (packageMatch) {
            currentPackage = packageMatch[1];
            continue;
        }

        // Check if the line contains benchmark information
        const benchmarkMatch = line.match(benchmarkRegex);
        if (benchmarkMatch?.groups) {
            const { name, procs, times, remainder } = benchmarkMatch.groups;
            const procsNumber = procs ? procs.slice(1) : null;
            const pieces = remainder.split(/\s+/);

            // For backward compatibility with Go benchmarks that had multiple metrics in the output,
            // but were not extracted properly before v1.18.0
            if (pieces.length > 2) {
                pieces.unshift(pieces[0], remainder.slice(remainder.indexOf(pieces[1])));
            }

            for (let i = 0; i < pieces.length; i += 2) {
                const value = parseFloat(pieces[i]);
                const unit = pieces[i + 1];
                const fullName = `${currentPackage}.${name}${i > 0 ? ' - ' + unit : ''}`;
                const extra = `${times} times${procsNumber ? `\n${procsNumber} procs` : ''}`.trim();
                results.push({ name: fullName, value, unit, extra });
            }
        }
    }

    return results;
}

I just don't know if the complete package changes the output too much you could use only part of the package name or write it in a separate property