Page MenuHomeFreeBSD

fread: improve performance for unbuffered reads
ClosedPublic

Authored by pfg on May 29 2021, 9:46 PM.

Details

Summary

We can use the buffer passed to fread(3) directly in the FILE *.
The buffer needs to be reset before each call to __srefill().
This preserves the expected behavior in all cases.

THe change was found originally in OpenBSD and later adopted by NetBSD.

Obtained from: OpenBSD (CVS 1.18)

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pfg requested review of this revision.May 29 2021, 9:46 PM

In principle, the optimization is perhaps useful for unbuffered consumers.

The locking in your patch is incompatible with the FreeBSD implementation. Test the patch in multithreaded program.

lib/libc/stdio/fread.c
127

This cannot be right.

In D30548#686024, @kib wrote:

In principle, the optimization is perhaps useful for unbuffered consumers.

The locking in your patch is incompatible with the FreeBSD implementation. Test the patch in multithreaded program.

Bah .. you are right, I am quite rusty on FreeBSD and I just copy-pasted. This shouldn't have compiled.

In D30548#686034, @pfg wrote:
In D30548#686024, @kib wrote:

In principle, the optimization is perhaps useful for unbuffered consumers.

The locking in your patch is incompatible with the FreeBSD implementation. Test the patch in multithreaded program.

Bah .. you are right, I am quite rusty on FreeBSD and I just copy-pasted. This shouldn't have compiled.

It should compile fine, but it cannot work.

What you need to do is to remove the wrong unlock line.

In D30548#686179, @kib wrote:
In D30548#686034, @pfg wrote:
In D30548#686024, @kib wrote:

In principle, the optimization is perhaps useful for unbuffered consumers.

The locking in your patch is incompatible with the FreeBSD implementation. Test the patch in multithreaded program.

Bah .. you are right, I am quite rusty on FreeBSD and I just copy-pasted. This shouldn't have compiled.

It should compile fine, but it cannot work.

What you need to do is to remove the wrong unlock line.

Remove or replace?

The change seems to have made an important difference in other BSDs but I'd honestly love to have someone that knows more about locking take over ;).

lib/libc/stdio/fread.c
127

Should it just be?

FLOCKFILE_CANCELSAFE(fp);
lib/libc/stdio/fread.c
127

I meant

FUNLOCKFILE_CANCELSAFE();

but I guess one has to lock before unlocking ?

lib/libc/stdio/fread.c
127

Look at fread() and __fread(). You patched __fread(), which assumes that the locks are taken externally and unlocked externally as well, when needed.

In other words, just delete (any) unlock you added.

Duh ! I see now.

Will fix soon. and run some more testing in my system. Thanks!

Remove deprecated unlocking, pointed out by kib

pfg marked 2 inline comments as done.May 31 2021, 1:52 AM

This looks fine to me, but I did not tested.

This revision is now accepted and ready to land.May 31 2021, 7:52 PM

Commit log nit: s/THe/The/