Compilation speed

[ccleve]

If you run "cargo pgrx run --release", it takes a long time to compile (as it should).

If you run it again, immediately, without making any code changes, it takes a long time. It hangs on this line:

Building [=======================> ] 231/232: pgrx_embed_relevantdb(bin)

I don't think it used to do that prior to 0.12.

I'm using 0.12.1.

[eeeebbbbrrrr]

Hey! Ya know, I've noticed this exact thing now that I've upgraded a project to 0.12. It's on my list to look at first thing next week. Off the top of my head, I am unsure what's going on.

[workingjubilee]

I'm guessing it's because we kiiinda "modify" the source in a way that cargo interprets as having diffed it...?

[usamoi]

It hangs if LTO is enabled. I usually build an optimized artifact with LTO disabled. I enable LTO only when I really need to release a new version.

[eeeebbbbrrrr]

I'm taking a look at it now

[eeeebbbbrrrr]

One quick experiment that looks good is for cargo-pgrx to not pass the --release or --profile XXX flags down when compiling the pgrx_embed binary -- instead always compile it in debug mode.

Immediately after a cargo clean, that causes two full compilations of the code if using --release/--profile, but after that, the built of the pgrx_embed binary is nearly instant.

Thoughts?

[eeeebbbbrrrr]

I put up PR #1838 that makes this a lot better. @ccleve / @usamoi, feel free to install cargo-pgrx from that PR if you want and see if it makes ya happier.

[workingjubilee]

setting LTO to fat will still be overkill, even for release binaries.

[ccleve]

Not passing the --release flag is a good solution.

[eeeebbbbrrrr]

@workingjubilee looking at this in some detail this morning brought up the memories of the 4 other ways pgrx has historically solved the schema generation problem.

I almost wonder if there's one more iteration here where we don't need the pg_embed.exe at all?

Why wouldn't it be possible to dlopen the extension.so with RTLD_LAZY and just directly call the function symbols we need to run?

From the manpage:

       RTLD_LAZY
              Perform lazy binding.  Resolve symbols only as the code
              that references them is executed.  If the symbol is never
              referenced, then it is never resolved.  (Lazy binding is
              performed only for function references; references to
              variables are always immediately bound when the shared
              object is loaded.)  Since glibc 2.1.1, this flag is
              overridden by the effect of the LD_BIND_NOW environment
              variable.

Maybe the "references to variables are always immediately bound" would be a blocker? I know you've put a lot of thought into this stuff too, so just wondering if you went down this path and threw it out?

[workingjubilee]

it ultimately results in arbitrary amounts of schema generation code being compiled into the shared object, like debuginfo you can't remove by applying strip.

[workingjubilee]

we also wouldn't be able to compile these functions without release opts, so LLVM would spend arbitrary amounts of time on each function optimizing a function we don't actually care about optimizing.

[workingjubilee]

like, I can't see how that's not just reverting #1468 ?

[eeeebbbbrrrr]

yeah, those are legit points. #hmm

I wonder if we could stash the PGRX_EMBED file into target/ somewhere as a not-as-transient version that we can then compare against the new version we're about to write to /tmp. If they're identical, we can skip the "second_build" and run the existing pg_embed.exe directly.

[workingjubilee]

this would probably be solved if cargo used content-addressable caching instead of file access times, and cargo is much better-positioned to solve this for us.

[eeeebbbbrrrr]

like, I can't see how that's not just reverting #1468 ?

Yeah, it probably effectively be that. Which is definitely not what we want.

[eeeebbbbrrrr]

this would probably be solved if cargo used content-addressable caching instead of file access times, and cargo is much better-positioned to solve this for us.

I've always wondered why they do it based on timestamps. Oh well.

[workingjubilee]

it seems mostly because these two haven't landed:

• Tracking Issue for checksum freshness rust-lang/cargo#14136
• Add unstable support for outputting file checksums for use in cargo rust-lang/rust#126930

[eeeebbbbrrrr]

The other downside to us compiling the extension code twice is if there's compilation warnings, the user seems them twice.

(That's actually part of my normal workflow -- letting the compiler keep up with warnings I want to address later.)

We could just redirect the output to /dev/null since we'd have never gotten to the pgrx_embed build if the actual extension compile failed.

Thoughts on that?

[workingjubilee]

which kinds of warnings are you thinking of?

I have been wondering if we should be replacing the insides of the non-schema functions with todo!() when compiling the schema binary.

[eeeebbbbrrrr]

which kinds of warnings are you thinking of?

Ya know, just compiler warnings like "unused variable" and the like. The general garbage that comes up while writing new code that compiles/runs but ain't finished yet.

[eeeebbbbrrrr]

non-schema functions

??? As in... all the rest of the code? How on earth would we do that?

[workingjubilee]

oh, easy. I mean, it wouldn't have 100% coverage, but most of the code is annotated with #[pg_extern].

[eeeebbbbrrrr]

I guess I don't see what that would accomplish? there's more code in an extension crate that our #[pg_XXX]-annotated functions

[workingjubilee]

sometimes half of something is better than none of it? and: faster compile times? /shrug

haven't evaluated it numerically.

[workingjubilee]

might be simpler to emit #![allow(warnings)] for just taking care of the warnings issue.

[eeeebbbbrrrr]

might be simpler to emit #![allow(warnings)] for just taking care of the warnings issue.

Oh. That’s a goood idea. I’ll try that now…

[eeeebbbbrrrr]

I would have expected this to work, in the extension's src/bin/pgrx_embed.rs file:

#![allow(warnings)]
::pgrx::pgrx_embed!();

but it does not. Still emits warnings:

       Using PgConfig("pg16") and `pg_config` from /home/pg/16/bin/pg_config
    Building extension with features pg16
     Running command "/home/zombodb/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/cargo" "build" "--lib" "--profile" "prof" "--features" "pg16" "--no-default-features" "--message-format=json-render-diagnostics"
warning: constant `foo` is never used
   --> xxx/src/lib.rs:184:7
    |
184 | const foo: i32 = 42;
    |       ^^^
    |
    = note: `#[warn(dead_code)]` on by default

warning: constant `foo` should have an upper case name
   --> xxx/src/lib.rs:184:7
    |
184 | const foo: i32 = 42;
    |       ^^^ help: convert the identifier to upper case (notice the capitalization): `FOO`
    |
    = note: `#[warn(non_upper_case_globals)]` on by default

warning: `xxx` (lib) generated 2 warnings
    Finished `prof` profile [optimized + debuginfo] target(s) in 0.14s
  Installing extension
     Copying control file to /home/pg/16/share/postgresql/extension/xxx.control
     Copying shared library to /home/pg/16/lib/postgresql/xxx.so
  Discovered 77 SQL entities: 0 schemas (0 unique), 73 functions, 2 types, 0 enums, 2 sqls, 0 ords, 0 hashes, 0 aggregates, 0 triggers
  Rebuilding pgrx_embed, in debug mode, for SQL generation with features pg16
warning: constant `foo` is never used
   --> xxx/src/lib.rs:184:7
    |
184 | const foo: i32 = 42;
    |       ^^^
    |
    = note: `#[warn(dead_code)]` on by default

warning: constant `foo` should have an upper case name
   --> xxx/src/lib.rs:184:7
    |
184 | const foo: i32 = 42;
    |       ^^^ help: convert the identifier to upper case (notice the capitalization): `FOO`
    |
    = note: `#[warn(non_upper_case_globals)]` on by default

warning: `xxx` (lib) generated 2 warnings
   Compiling xxx v0.9.3 (/home/zombodb/_work/xxx/xxx)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.50s
     Writing SQL entities to /home/pg/16/share/postgresql/extension/xxx--0.9.3.sql
    Finished installing xxx

I also experimented with changing cargo-pgrx to add -- -A warnings to the cargo rustc Command it execs, but that didn't work either.

I'm starting to believe that directive doesn't actually do anything.

[eeeebbbbrrrr]

I'm starting to believe that directive doesn't actually do anything.

I guess the directive isn't being pushed down to dependencies?

Like, I suppose the bin/pgrx_embed.rs is its own module, of sorts, so while #![allow(warnings)] applies to it, it doesn't apply to the extension itself, which is a different module. I bet that's what is going on here.

Oh well. I can live with it.

[workingjubilee]

Hmm, probably so.

[eeeebbbbrrrr]

Closed by #1838.