845907 - Generic readahead functions

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Write some generic functions that abstract out readahead for Windows, OSX, and Linux. Modify XPCOM standalone glue to call into these. I'm migrating patch 1 from bug 810454 over to here.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Proposed patch

Comment 2

[(dormant account)]

Comment on attachment 719298 [details] [diff] [review]
Proposed patch

nit:
+mozilla::PreloadLib(nsIFile* aFile)

s/PreloadFile/readahead/ since we are already using unixy convention for fallocate

Note in the future we may want to modify the signature to return a filehandle so we can pass it to sqlite, etc.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 719298 [details] [diff] [review]
Proposed patch

Review of attachment 719298 [details] [diff] [review]:
-----------------------------------------------------------------

Whatever name we end up with (cf. Taras's comment), I think you should use function overloading instead of having a something and somethingStr variants.

One concern I have is that we need to be extra careful with these functions, as they are effectively blocking on I/O. I actually wonder if we shouldn't make them MOZ_ASSERT on not being on the main thread (except in the standalone glue case).

r- just because I want to see next iteration.

::: xpcom/glue/FileUtils.cpp
@@ +148,5 @@
> +}
> +
> +#endif // XPCOM_GLUE
> +
> +#if defined(LINUX) && !defined(ANDROID)

You can lift the !defined(ANDROID) here. But that may require a custom wrapper for the readahead syscall ; I think bionic lacks one. Or we can do that in a followup.

@@ +382,5 @@
> +    }
> +    cmd += sh->cmdsize;
> +  }
> +  // Let the kernel read ahead what the dynamic loader is going to
> +  // map in memory soon after. The F_RDADVISE fcntl is equivalent

You may want o move the F_RDADVISE comment :)

::: xpcom/glue/FileUtils.h
@@ +22,4 @@
>  
>  namespace mozilla {
>  
> +#if !defined(XPCOM_GLUE)

Please add a comment that functions not to be used in the standalone glue go in this block.

@@ +120,5 @@
> + *
> + * @param aFd file descriptor opened for read access
> + * (on Windows, file must be opened with FILE_FLAG_SEQUENTIAL_SCAN)
> + * @param aOffset Offset into the file to begin preloading
> + * @param aCount Number of bytes to preload (SIZE_MAX implies file size)

Please document that the function doesn't change the position. (that is, that preload(fd); read(fd, buf, len); will read from the start of the file)

Comment 4

[Mike Hommey [:glandium]]

(In reply to Taras Glek (:taras) from comment #2)
> Comment on attachment 719298 [details] [diff] [review]
> Proposed patch
> 
> nit:
> +mozilla::PreloadLib(nsIFile* aFile)
> 
> s/PreloadFile/readahead/

ReadAhead, then.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch rev 2

(In reply to Mike Hommey [:glandium] from comment #3)
> Whatever name we end up with (cf. Taras's comment), I think you should use
> function overloading instead of having a something and somethingStr variants.

Agreed, done.

> One concern I have is that we need to be extra careful with these functions,
> as they are effectively blocking on I/O. I actually wonder if we shouldn't
> make them MOZ_ASSERT on not being on the main thread (except in the
> standalone glue case).

I agree with this in principle, however I don't think that we can do it yet. In the short term, many of the use cases for these functions are to reduce the impact of existing main thread I/O.

> ::: xpcom/glue/FileUtils.cpp
> @@ +148,5 @@
> > +}
> > +
> > +#endif // XPCOM_GLUE
> > +
> > +#if defined(LINUX) && !defined(ANDROID)
> 
> You can lift the !defined(ANDROID) here. But that may require a custom
> wrapper for the readahead syscall ; I think bionic lacks one. Or we can do
> that in a followup.

Followup? I'd like to learn a bit more about this first.

> @@ +382,5 @@
> > +    }
> > +    cmd += sh->cmdsize;
> > +  }
> > +  // Let the kernel read ahead what the dynamic loader is going to
> > +  // map in memory soon after. The F_RDADVISE fcntl is equivalent
> 
> You may want o move the F_RDADVISE comment :)

Oops! ;) Done.


> ::: xpcom/glue/FileUtils.h
> @@ +22,4 @@
> >  
> >  namespace mozilla {
> >  
> > +#if !defined(XPCOM_GLUE)
> 
> Please add a comment that functions not to be used in the standalone glue go
> in this block.

Done.

> @@ +120,5 @@
> > + *
> > + * @param aFd file descriptor opened for read access
> > + * (on Windows, file must be opened with FILE_FLAG_SEQUENTIAL_SCAN)
> > + * @param aOffset Offset into the file to begin preloading
> > + * @param aCount Number of bytes to preload (SIZE_MAX implies file size)
> 
> Please document that the function doesn't change the position. (that is,
> that preload(fd); read(fd, buf, len); will read from the start of the file)

Done.

Also, I added an optional out parameter to the ReadAheadFile functions to return the fd instead of closing it, as per Taras' remarks.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 719581 [details] [diff] [review]
Patch rev 2

Review of attachment 719581 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following nits.

::: xpcom/glue/FileUtils.cpp
@@ +38,3 @@
>  
>  bool 
>  mozilla::fallocate(PRFileDesc *aFD, int64_t aLength) 

Can you remove the trailing spaces on these two lines while you're here?

@@ +131,5 @@
> +#endif
> +}
> +
> +void
> +mozilla::ReadAheadFile(nsIFile* aFile, mozilla::filedesc_t* aOutFd,

Make the out param the last.

@@ +242,5 @@
> +  fpOffset.u.HighPart = 0;
> +#endif
> +
> +  // Get the current file pointer so that we can restore it. This isn't 
> +  // really necessary other than to provide the same semantics regarding the 

Please remove the trailing spaces on these two lines.

@@ +275,5 @@
> +  }
> +
> +  // Restore the file pointer
> +  SetFilePointerEx(aFd, fpOriginal, nullptr, FILE_BEGIN);
> +#elif defined(LINUX) && !defined(ANDROID)

Maybe add some empty lines around the #elifs to make this more legible.

@@ +282,5 @@
> +  struct radvisory ra;
> +  ra.ra_offset = aOffset;
> +  ra.ra_count = aCount;
> +  // Let the kernel read ahead what the dynamic loader is going to
> +  // map in memory soon after. The F_RDADVISE fcntl is equivalent

The part about the dynamic loader is for ReadAheadLib :)

@@ +395,5 @@
> +}
> +
> +void
> +mozilla::ReadAheadFile(mozilla::pathstr_t aFilePath,
> +                       mozilla::filedesc_t* aOutFd,

Make the out param last.

::: xpcom/glue/FileUtils.h
@@ +123,5 @@
> +                               const size_t aCount = SIZE_MAX);
> +
> +/**
> + * Use readahead to preload a file into the file cache before reading.
> + * When this function exits, the file pointer is guaranteed to be in the same 

trailing space.

::: xpcom/glue/standalone/nsGlueLinkingDlopen.cpp
@@ +10,3 @@
>  
>  #if defined(LINUX) && !defined(ANDROID)
>  #define _GNU_SOURCE 

Can you remove the trailing space on that line as well?

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a40244b87c7a

Try builds for this patch:
https://tbpl.mozilla.org/?tree=Try&rev=6432c2a7ee04
https://tbpl.mozilla.org/?tree=Try&rev=5f337f0f188e
(The second one is to ensure that platforms without readahead support still compile)

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Backed out due to some B2G bustage
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6fef674318

Comment 9

[(dormant account)]

(In reply to Mike Hommey [:glandium] from comment #3)

> One concern I have is that we need to be extra careful with these functions,
> as they are effectively blocking on I/O. I actually wonder if we shouldn't
> make them MOZ_ASSERT on not being on the main thread (except in the
> standalone glue case).

It's ok to block on IO if the IO that follows is on main thread, we'll need to use this for addon manager, other places.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Taras Glek (:taras) from comment #9)
> It's ok to block on IO if the IO that follows is on main thread, we'll need
> to use this for addon manager, other places.

It depends if you replace a 100ms IO with a 1s one.

Comment 11

[(dormant account)]

(In reply to Mike Hommey [:glandium] from comment #10)
> (In reply to Taras Glek (:taras) from comment #9)
> > It's ok to block on IO if the IO that follows is on main thread, we'll need
> > to use this for addon manager, other places.
> 
> It depends if you replace a 100ms IO with a 1s one.

We should require telemetry to prove that readahead is helping... Maybe mention in function docs, that usage of this functionality should not go in without a telemetry field trial

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch rev 3

Patch fixes nits, simple build fix for b2g, carrying forward r+

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch rev 4

Added cautionary comments to function docs as Taras suggested. Carrying forward r+

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/779b7ec8c9ec

Try build:
https://tbpl.mozilla.org/?tree=Try&rev=0aefc2698bcb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/779b7ec8c9ec