If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Generic readahead functions

RESOLVED FIXED in mozilla22

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Created attachment 719298 [details] [diff] [review]
Proposed patch
Attachment #719298 - Flags: review?(mh+mozilla)

Comment 2

5 years ago
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 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)
Attachment #719298 - Flags: review?(mh+mozilla) → review-
(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.
Created attachment 719581 [details] [diff] [review]
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.
Attachment #719298 - Attachment is obsolete: true
Attachment #719581 - Flags: review?(mh+mozilla)
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?
Attachment #719581 - Flags: review?(mh+mozilla) → review+
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)
Backed out due to some B2G bustage
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6fef674318

Comment 9

5 years ago
(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.
(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

5 years ago
(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
Created attachment 720495 [details] [diff] [review]
Patch rev 3

Patch fixes nits, simple build fix for b2g, carrying forward r+
Attachment #719581 - Attachment is obsolete: true
Attachment #720495 - Flags: review+
Created attachment 720496 [details] [diff] [review]
Patch rev 4

Added cautionary comments to function docs as Taras suggested. Carrying forward r+
Attachment #720495 - Attachment is obsolete: true
Attachment #720496 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/779b7ec8c9ec

Try build:
https://tbpl.mozilla.org/?tree=Try&rev=0aefc2698bcb
https://hg.mozilla.org/mozilla-central/rev/779b7ec8c9ec
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 911425
You need to log in before you can comment on or make changes to this bug.