Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mark pgrx_pg_sys bindings inline #1616

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

usamoi
Copy link
Contributor

@usamoi usamoi commented Mar 18, 2024

I did a little investigation in compilation time and found the most time-consuming part in pgrx is rustc codegen: most bindings of PostgreSQL are not used but rustc spends time in generating machine code of pg_guard wrappers. I just read https://kobzol.github.io/rust/rustc/2024/03/15/rustc-what-takes-so-long.html and it said, Because if you just build an intermediate artifact (like an .rlib, which is what your crate dependencies compile into), that won’t compile #[inline]-d and generic functions, and also the linker won’t be involved. So, I thought #[inline] should help compilation time.

Marking pgrx_pg_sys bindings inline reduces compilation time. It took 150 seconds to build pgrx before and it takes 36 seconds to build pgrx now.

cargo build --no-default-features --features pg13 --release --timings

Before this PR:

Screenshot_20240318_184237

After this PR:

Screenshot_20240318_183904

The “codegen” times are highlighted in a lavender color.
The “custom build” units are build.rs scripts, which when run are highlighted in orange.

Inlining prevents unused codegen in macros, so it's faster.

Signed-off-by: usamoi <usamoi@outlook.com>
@eeeebbbbrrrr
Copy link
Contributor

Wow. That’s such an improvement that I kinda don’t believe it.

Are those both timings with “cargo clean”?

@usamoi
Copy link
Contributor Author

usamoi commented Mar 18, 2024

Are those both timings with “cargo clean”?

Yes.

@eeeebbbbrrrr
Copy link
Contributor

I’m gonna go ahead and merge. Would you mind linking that blog post you linked on discord here in this PR too?

@eeeebbbbrrrr eeeebbbbrrrr merged commit 0c62e04 into pgcentralfoundation:develop Mar 18, 2024
11 checks passed
@usamoi
Copy link
Contributor Author

usamoi commented Mar 18, 2024

Would you mind linking that blog post you linked on discord here in this PR too?

It's beneficial.

@eeeebbbbrrrr
Copy link
Contributor

Apparently this came about fromm https://kobzol.github.io/rust/rustc/2024/03/15/rustc-what-takes-so-long.html

Specifically this sentence:

Because if you just build an intermediate artifact (like an .rlib, which is what your crate dependencies compile into), that won’t compile #[inline]-d and generic functions, and also the linker won’t be involved

@eeeebbbbrrrr
Copy link
Contributor

BTW, @usamoi… thanks. This is a huge win.

@workingjubilee
Copy link
Member

Ah, this is because of an artifact of rustc that is somewhat dependent on how things currently work, rather than something necessarily true, though I suppose we can always revert it later if things do change significantly.

Namely, when a function is inline, currently rustc defers codegen into the rlib until it reaches the actual callsite.

@usamoi usamoi deleted the inline branch March 20, 2024 13:54
eeeebbbbrrrr added a commit that referenced this pull request Apr 19, 2024
(cherry-picking that commit caused a number of conflicts due to other drift in the develop branch)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants