Add target column, defaulting to x86_64-unknown-linux-gnu

[Jamesbarford]

The PR adds a compiler_target column to the pstat_series table. As well as passing through the compiler_target; currently it is CompilerTarget::default() which is set to x86_64-unknown-linux-gnu.

[Kobzol]

Thank you! I would rename it to CompileTarget, as compiler target is not a terminology that I know from anywhere and it could be a bit misleading.

I thought that using a String is fine for this, but now that I'm thinking about it, using an enum would have a non-trivial advantage in that it would be more efficient, especially memory wise. The Index and the queries go through tens of thousands of commits, and the webserver machine is not very beefy, so I would like to ask you to turn the target into an enum, which should take way less memory (and also be faster).

It will also make it easier to generate CLI help for selecting targets, as we will have a known set of targets.

[Jamesbarford]

Thank you! I would rename it to CompileTarget, as compiler target is not a terminology that I know from anywhere and it could be a bit misleading.

I thought that using a String is fine for this, but now that I'm thinking about it, using an enum would have a non-trivial advantage in that it would be more efficient, especially memory wise. The Index and the queries go through tens of thousands of commits, and the webserver machine is not very beefy, so I would like to ask you to turn the target into an enum, which should take way less memory (and also be faster).

It will also make it easier to generate CLI help for selecting targets, as we will have a known set of targets.

Thanks!

With respect to the enum being created, is this to make the column in the Postgres table an enum, in the Rust code an enum or both?

[Kobzol]

I meant to handle it in the same way as we do for the other enums, e.g. Profile. So store it as an enum in Rust code, but still a normal string/TEXT in the DB (pstat_series).

(btw I left you some messages on Zulip regarding the naming :) )

[Kobzol]

Thanks, left a few nits.

It would be nice to also modify the postgres-to-sqlite.rs and sqlite-to-postgres.rs binaries in the database crate. It's not super important though, I can do it later if needed.

[Kobzol]

Thanks! Have you tried to gather benchmarks locally and visualize them in the local website? Just to check that this works. I will also try it locally later.

[Jamesbarford]

Thanks! Have you tried to gather benchmarks locally and visualize them in the local website? Just to check that this works. I will also try it locally later.

I ran the migration locally on a SQLite database I downloaded using the fetch-latest tool. The data looked to display ok and have no visual changes from what I had seen previously when running the application.

[Kobzol]

Just a note that might be useful going forward: when performing DB changes, it's also useful to test it on Postgres, because the two DB implementations are completely separate.

I use the following docker-compose.yml file:

version: '3'
services:
    postgres:
        image: 'postgres:15.3'
        ports:
            - '5432:5432'
        environment:
            POSTGRES_PASSWORD: 'postgres'
            POSTGRES_USER: 'postgres'
            POSTGRES_DB: 'perf'

and then specify the connection string using the --db parameter:

$ cargo run --bin collector bench_local `rustup +nightly which rustc` --include helloworld --db postgresql://postgres:[email protected]:5432/perf

I tested this on Postgres and it looks fine.

[Kobzol]

Suggested change

	/// Target representing an Rust target tripple, for a full list of targets and
	/// Target representing an Rust target triple, for a full list of targets and

[Kobzol]

Could you please also keep the target before metric here, to keep metric at the end?

[Kobzol]

Suggested change

	.map(|(_, _, _, _, metric, _)| metric)
	.map(|(_, _, _, _, _, metric)| metric)

Otherwise it would actually select the target.

[Kobzol]

Suggested change

	Target::from_str(row.get::<_, String>(6).as_str()).unwrap(),
	Target::from_str(row.get::<_, String>(5).as_str()).unwrap(),

And the line below need to use (6).

[Kobzol]

Thank you! I think that this is ready now, but I will hold off merging until Monday, just to avoid deploying on a Friday evening =D

[Kobzol]

Actually, CI is not green yet.

[Jamesbarford]

Actually, CI is not green yet.

The error; table pull_request_build has no column named backends suggests I may have messed up the database migrations 😬. Could this be due to when I accidentally removed a migration from the Vec of migrations?

[Kobzol]

Ah, yes, here, you deleted an old migration.

[Kobzol]

Thank you! :)