jobserver-rs icon indicating copy to clipboard operation
jobserver-rs copied to clipboard

Add back `preadv2` optimization for `try_acquire` on Linux

Open NobodyXu opened this issue 10 months ago • 13 comments

Fixed #87

NobodyXu avatar Apr 22 '24 13:04 NobodyXu

cc @the8472 can you check the syscall I added please?

I'm not sure if it is correct, thank you!

NobodyXu avatar Apr 22 '24 13:04 NobodyXu

what are the numbers on this optimization like?

It would enable try_acquire support on cases where:

  • jobserver gets an annoymous pipe instead of named fifo
  • procfs is not available Without preadv2, try_acquire would return a not-supported error.

On performance, it's mostly for avoiding performance loss with acquire when jobserver receives a named fifo or procfs is available (which jobserver would use to convert pipe to fifo).

Without preadv2,try_acquire would set the fifo to non-blocking, which would cause acquire to enter poll + read if the fifo is empty, which is less efficient than a blocking read.

NobodyXu avatar Apr 23 '24 12:04 NobodyXu

Maybe we can simplify things by limiting to aarch64, x86_64 linux gnu? Those are probably the most common environments where stuff is built.

Well, from the glibc implementation, it looks like there're only 3 versions for it, so maybe we can still bare with the cost?

And it's mostly a perf optimization, so we can be conservative in which platforms get it.

While it's mostly a perf optimization, there is situation where preadv2 actually enable try_acquire to work properly instead of returning a not-supported error:

  • jobserver gets an annoymous pipe instead of named fifo
  • procfs is not available Without preadv2, try_acquire would return a not-supported error.

NobodyXu avatar Apr 23 '24 12:04 NobodyXu

what are the numbers on this optimization like?

It would enable try_acquire support on cases where:

* jobserver gets an annoymous pipe instead of named fifo

* procfs is not available
  Without `preadv2`, `try_acquire` would return a not-supported error.

On performance, it's mostly for avoiding performance loss with acquire when jobserver receives a named fifo or procfs is available (which jobserver would use to convert pipe to fifo).

Without preadv2,try_acquire would set the fifo to non-blocking, which would cause acquire to enter poll + read if the fifo is empty, which is less efficient than a blocking read.

Thanks @NobodyXu for the work. Believing that people already know the benefit of this for fixing unsupported situations. What Jubilee asked for are numbers. I think that's a reasonable request for a change aiming at optimization. Could you do some benchmark and show the number, so we can have a baseline in mind? It is also beneficial to prevent from future performance regression if we have the reproducible benchmark steps (if you don't mind sharing).

weihanglo avatar Apr 25 '24 12:04 weihanglo

What Jubilee asked for are numbers. I think that's a reasonable request for a change aiming at optimization. Could you do some benchmark and show the number, so we can have a baseline in mind? It is also beneficial to prevent from future performance regression if we have the reproducible benchmark steps (if you don't mind sharing).

I could add one in benches/, it would first call try_acquire on Linux and then call acquire. Might have to add criterion as a dev-dep though.

Edit:

Would do that tomorrow.

NobodyXu avatar Apr 25 '24 12:04 NobodyXu

Note that using blocking reads in acquire is mostly beneficial when there are many concurrent waiters. A single waiter won't see much of a speedup (perhaps a little from the avoided syscalls).

See #30 and https://github.com/torvalds/linux/commit/0ddad21d3e99c743a3aa473121dc5561679e26bb, the latter one has numbers.

the8472 avatar Apr 25 '24 15:04 the8472

Oh, cute, so it is in fact a kernel optimization specifically for this protocol!

workingjubilee avatar Apr 27 '24 03:04 workingjubilee

Can the code generally minimize amount of #[cfg(...)] in favor of cfg!(...)? It will get more checking and less surprises on remaining platforms. #[cfg(...)] is not really needed that often, only when some names are unavailable on some platforms (and even then they can sometimes be defined to dummies, see e.g. how type RawFd is defined).

petrochenkov avatar Apr 27 '24 12:04 petrochenkov

Can the code generally minimize amount of #[cfg(...)] in favor of cfg!(...)? It will get more checking and less surprises on remaining platforms.

Thanks, I've added the code in preadv2 to use cfg! instead of #[cfg(...)], it's actually more readable now.

NobodyXu avatar Apr 29 '24 13:04 NobodyXu

Yeah I would add one, I wish libc would support it for musl libc

Or perhaps we could wait till libc support it on musl @the8472 ?

NobodyXu avatar Jun 28 '24 03:06 NobodyXu

Yeah it's not urgent. try_acquire is new and I assume not many things are using it yet so the negative performance impact of making the pipe non-blocking isn't important yet.

the8472 avatar Jun 28 '24 08:06 the8472

try_acquire is new and I assume not many things are using it yet so the negative performance impact of making the pipe non-blocking isn't important yet.

Well cc is using it so it does have some effect to the ecosystem as a whole, though depending on the project, it might not be that large as compiling is much more expensive.

NobodyXu avatar Jun 28 '24 08:06 NobodyXu

Opened rust-lang/libc#3760

NobodyXu avatar Jun 28 '24 08:06 NobodyXu