Asynchronously pre-fault the native library pages.

Asynchronously pre-fault the native library pages.

This can lead to an improvement up to 350ms out of 3400 for "time to navigation commit".
See the linked bug for the details.


BUG=460438

[Benoit L, 2015-03-02 09:27:17]

lizeb@chromium.org changed reviewers:
+ primiano@chromium.org, simonb@chromium.org

[Benoit L, 2015-03-02 09:27:17]

Hello Simon, Primiano,

Here is the CL adding pre-faulting for the native libraries used in Chrome on
Android. It is done after the native libraries have been loaded because this has
been found as being the most useful in local experiments.

For background and experimental results, see the linked bug (in particular, the
ipython notebook attached to the first update).

Thanks!

[Primiano Tucci (use gerrit), 2015-03-03 20:03:07]

I had some initial discussion with Torne and Simon.

At a first glance, it feels like most of the code inside here can be avoided
leveraging just the symbols that the linker gives you for free.
You want to double check them, but the rought idea is:

// These two magic symbols give are relocated by the linker and should give the
start and end that you need.
extern "C" char __executable_start; 
extern "C" char etext;

uintptr_t start = (uintptr_t) &__executable_start
uintptr_t end = (uintptr_t) &etext
for (i = start; i < end; i += 4096) { ...read... }

without even knowing about the existance of the crazylinker or worrying whether
the library is mapped from file or from a zip.

Another question: once you will get the orderfile, it should make sense just to
fault the first X Mb of the binary, not the entire thing, right? That should
give you most of the speedup.

I still oersonally support the idea of doing this pre-faulting in a forked
process (and making it suicide immediately after). 
Doesn't need to be necessarily part of this cl. But that IMHO will avoid the
risk of miserably crashing chrome  if something in the linker map or the
crazylinker goes wrong. 
Also, by virtue of forking, you will not end up inflating the RSS of the process
(well, you will do, but for a process that you are going to murder immediately
after).

[chromium-reviews, 2015-03-04 10:05:28]

On Tue, Mar 3, 2015 at 9:03 PM, <primiano@chromium.org> wrote:

> I had some initial discussion with Torne and Simon.
>
> At a first glance, it feels like most of the code inside here can be
> avoided
> leveraging just the symbols that the linker gives you for free.
> You want to double check them, but the rought idea is:
>
> // These two magic symbols give are relocated by the linker and should
> give the
> start and end that you need.
> extern "C" char __executable_start;
> extern "C" char etext;
>
> uintptr_t start = (uintptr_t) &__executable_start
> uintptr_t end = (uintptr_t) &etext
> for (i = start; i < end; i += 4096) { ...read... }
>
> without even knowing about the existance of the crazylinker or worrying
> whether
> the library is mapped from file or from a zip.
>
>
If I am not mistaken, this would only give the start/end of the .text
section. However, the .data section is also important in this case. Also,
as discussed by chat with Primiano, I believe that it is better to keep the
pages not referenced, so that they are the first one being evicted by the
kernel in case of memory pressure. These two reasons made me lean towards
making a separate mapping instead of directly touching the library.

If I understand correctly, the concern here is that crazylinker is already
crazy enough, and that we shouldn't add code to it.
Most of the code can move out of crazylinker, the only part that is needed
is a way to get the offset/size of a lib within the ZIP file.



> Another question: once you will get the orderfile, it should make sense
> just to
> fault the first X Mb of the binary, not the entire thing, right? That
> should
> give you most of the speedup.
>

Yes and no. Yes because we expect most pages that are accessed to be at the
beginning of the file. No because we are probably missing something in the
orderfile, so it is better to map everything. Also, the orderfile doesn't
cover the .data section.


>
> I still oersonally support the idea of doing this pre-faulting in a forked
> process (and making it suicide immediately after).
>

AFAIK, fork() is not in the NDK. And I would prefer to avoid hacks (I know,
we are already talking about crazylinker ;-) )


> Doesn't need to be necessarily part of this cl. But that IMHO will avoid
> the
> risk of miserably crashing chrome  if something in the linker map or the
> crazylinker goes wrong.
> Also, by virtue of forking, you will not end up inflating the RSS of the
> process
> (well, you will do, but for a process that you are going to murder
> immediately
> after).
>
> https://codereview.chromium.org/958473003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Torne, 2015-03-04 10:21:50]

The linker defines lots of magic symbols that have the starts/ends of
various sections, not just .text, and fork() is in the NDK. Using fork on
android is strongly discouraged because the child process is not tracked by
the activity manager and thus not part of the OOM management scheme, but it
works and for a very short lived child it is probably not an issue.

On Wed, 4 Mar 2015 10:05 am Benoit Lize <lizeb@google.com> wrote:

> On Tue, Mar 3, 2015 at 9:03 PM, <primiano@chromium.org> wrote:
>
>> I had some initial discussion with Torne and Simon.
>>
>> At a first glance, it feels like most of the code inside here can be
>> avoided
>> leveraging just the symbols that the linker gives you for free.
>> You want to double check them, but the rought idea is:
>>
>> // These two magic symbols give are relocated by the linker and should
>> give the
>> start and end that you need.
>> extern "C" char __executable_start;
>> extern "C" char etext;
>>
>> uintptr_t start = (uintptr_t) &__executable_start
>> uintptr_t end = (uintptr_t) &etext
>> for (i = start; i < end; i += 4096) { ...read... }
>>
>> without even knowing about the existance of the crazylinker or worrying
>> whether
>> the library is mapped from file or from a zip.
>>
>>
> If I am not mistaken, this would only give the start/end of the .text
> section. However, the .data section is also important in this case. Also,
> as discussed by chat with Primiano, I believe that it is better to keep the
> pages not referenced, so that they are the first one being evicted by the
> kernel in case of memory pressure. These two reasons made me lean towards
> making a separate mapping instead of directly touching the library.
>
> If I understand correctly, the concern here is that crazylinker is already
> crazy enough, and that we shouldn't add code to it.
> Most of the code can move out of crazylinker, the only part that is needed
> is a way to get the offset/size of a lib within the ZIP file.
>
>
>
>> Another question: once you will get the orderfile, it should make sense
>> just to
>> fault the first X Mb of the binary, not the entire thing, right? That
>> should
>> give you most of the speedup.
>>
>
> Yes and no. Yes because we expect most pages that are accessed to be at
> the beginning of the file. No because we are probably missing something in
> the orderfile, so it is better to map everything. Also, the orderfile
> doesn't cover the .data section.
>
>
>>
>> I still oersonally support the idea of doing this pre-faulting in a forked
>> process (and making it suicide immediately after).
>>
>
> AFAIK, fork() is not in the NDK. And I would prefer to avoid hacks (I
> know, we are already talking about crazylinker ;-) )
>
>
>> Doesn't need to be necessarily part of this cl. But that IMHO will avoid
>> the
>> risk of miserably crashing chrome  if something in the linker map or the
>> crazylinker goes wrong.
>> Also, by virtue of forking, you will not end up inflating the RSS of the
>> process
>> (well, you will do, but for a process that you are going to murder
>> immediately
>> after).
>>
>> https://codereview.chromium.org/958473003/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Primiano Tucci (use gerrit), 2015-03-04 10:53:43]

> Also,
> as discussed by chat with Primiano, I believe that it is better to keep the
> pages not referenced, so that they are the first one being evicted by the
> kernel in case of memory pressure. 

That is precisely the reason why I was suggesting to fork().
If you do the trick in the forked process, they will be referenced only for the
time that takes to your process to run and once you terminate it (i.e. after the
loop) they will be unreferenced again.

> AFAIK, fork() is not in the NDK. 
AFAIK we use fork (actually, clone()) every single time crhome crashes and
breakpad kicks in.
See the breakpad/src/client/linux/handler/exception_handler.cc line 489

[chromium-reviews, 2015-03-04 10:57:00]

Thank you for the feedback. I will iterate on the patch then, no need to
review this version.

On Wed, Mar 4, 2015 at 11:53 AM, <primiano@chromium.org> wrote:

> Also,
>> as discussed by chat with Primiano, I believe that it is better to keep
>> the
>> pages not referenced, so that they are the first one being evicted by the
>> kernel in case of memory pressure.
>>
>
> That is precisely the reason why I was suggesting to fork().
> If you do the trick in the forked process, they will be referenced only
> for the
> time that takes to your process to run and once you terminate it (i.e.
> after the
> loop) they will be unreferenced again.
>
>  AFAIK, fork() is not in the NDK.
>>
> AFAIK we use fork (actually, clone()) every single time crhome crashes and
> breakpad kicks in.
> See the breakpad/src/client/linux/handler/exception_handler.cc line 489
>
> https://codereview.chromium.org/958473003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.