811671 - Reduce private pages due to relocations to shared libraries

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P2)

Description

[Ting-Yuan Huang]

Attachment: memory snapshot

Attached is a snapshot from a recently built unagi. There are around 2MB private RW pages spent on shared libraries for each process. Most of them are used for dynamic relocations. It would be helpful to share those pages.

After discussing with Thinker, we would like to change the way plugin-container instances created a little bit. Currently the instances are created by fork(), followed by exec() and the memory mappings are destroyed. If we fork() + exec() only the first plugin-container instance that forks the remaining without exec(), the new instances would be saved from relocations and be able to share those pages.

Another way would be prelink but it seems to require more efforts, although it may avoid PICs and make the code a little bit smaller.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Interesting!

(In reply to Ting-Yuan Huang from comment #0)
> Created attachment 681419 [details]
> After discussing with Thinker, we would like to change the way
> plugin-container instances created a little bit. Currently the instances are
> created by fork(), followed by exec() and the memory mappings are destroyed.
> If we fork() + exec() only the first plugin-container instance that forks
> the remaining without exec(), the new instances would be saved from
> relocations and be able to share those pages.
> 

We've considered this before, but only in the context of speeding up startup time, not for reducing memory usage.  Unfortunately this approach doesn't improve startup time over our current "prelaunch process" so we shelved this work.

Implementing this would require a very large, invasive and risky patch in our current code.  It would also create risk for desktop Firefox so would be hard to backport to aurora.  I'd like to exhaust other options before we go down this path.

> Another way would be prelink but it seems to require more efforts, although
> it may avoid PICs and make the code a little bit smaller.

That would kill ASLR but I think this level of win would be worth that.  We would stay at parity with android on this class of devices.

mwu/glandium, how hard would it be to set up the prelink magic for plugin-container?  (I guess b2g, too.)

Comment 2

[Mike Hommey [:glandium]]

It should only be a matter of running the prelink program on the b2g libs.

Comment 3

[Justin Lebar (not reading bugmail)]

I would really love a simple change which saves 2mb per child process.

Comment 4

[Mike Hommey [:glandium]]

Note I doubt it will actually save 2mb per child process, because relocations is not the only thing that makes data pages not being shared between processed. It will, however, save the relocated pages of the .data.rel.ro section.

Comment 5

[Ting-Yuan Huang]

You are right. I underestimated .data and .bss. The sum of .data.rel.ro and .data.rel.ro.local (this could also be saved, right?) over all b2g shared objects is about 1.5MB. The remaining system libraries linked, in the case of plugin-container, should contribute less than 500KB, perhaps 100KB ~ 300KB I guess.

Good news is that I also forgot to count in the benefits from other processes such as rild, if prelink were used.

On the other hand, apriori, the prelinker of Android, is unable to resolve global references. It also has problem when encountering GNU_versym that appears in libxul.so. I'll try to fix it.

Comment 6

[Ting-Yuan Huang]

Attachment: some statistics

I tried a slightly different approach to prelink. The dynamic linker was changed to cache relocations which can be shared among processes. In contrast to prelink, ASLR can be retained to some extent. Foreign libraries and executables could also be benefited.

Attached are memory reports with and without caching. Also attached are smaps of b2g(main process) and plugin-container(calendar). Pages mapping to "/system/tmp/lib*.so" are caches. There are 1384KB private dirty pages turned into private clean.

There are some more improvements to do in linker script:
1. ".data.rel.ro.local" could also be cached. It is 118KB in libxul.so
2. The start and end pages of ".data.rel.ro" are shared with other sections and thus could not be shared among different processes.

It seems that all b2g stuffs are linked by the toolchain's built-in linker script. Should we link them by android's linker script? That is "build/core/armelf*.x*".

Comment 7

[Mike Hommey [:glandium]]

(In reply to Ting-Yuan Huang from comment #6)
> I tried a slightly different approach to prelink. The dynamic linker was
> changed to cache relocations which can be shared among processes. In
> contrast to prelink, ASLR can be retained to some extent. Foreign libraries
> and executables could also be benefited.

How do you share the relocated parts of the libs between different processes?

Comment 8

[Ting-Yuan Huang]

(In reply to Mike Hommey [:glandium] from comment #7)
> (In reply to Ting-Yuan Huang from comment #6)
> > I tried a slightly different approach to prelink. The dynamic linker was
> > changed to cache relocations which can be shared among processes. In
> > contrast to prelink, ASLR can be retained to some extent. Foreign libraries
> > and executables could also be benefited.
> 
> How do you share the relocated parts of the libs between different processes?

For the ease of debugging, currently I write them to /system/tmp/* (only relocated parts) and mmap() them when needed. I'll later change to ashmem and maybe have a centralized daemon for security. Any ideas?

Comment 9

[Mike Hommey [:glandium]]

(In reply to Ting-Yuan Huang from comment #8)
> (In reply to Mike Hommey [:glandium] from comment #7)
> > (In reply to Ting-Yuan Huang from comment #6)
> > > I tried a slightly different approach to prelink. The dynamic linker was
> > > changed to cache relocations which can be shared among processes. In
> > > contrast to prelink, ASLR can be retained to some extent. Foreign libraries
> > > and executables could also be benefited.
> > 
> > How do you share the relocated parts of the libs between different processes?
> 
> For the ease of debugging, currently I write them to /system/tmp/* (only
> relocated parts) and mmap() them when needed.

That's going to be slow. That's definitely nice for prototyping, but not for production :)

> I'll later change to ashmem and maybe have a centralized daemon for security. Any ideas?

That's basically what i'm doing with our custom linker on android. Anyways, for immediate gains for v1, prelinking is the way to go.

Comment 10

[Thinker Li [:sinker]]

(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Ting-Yuan Huang from comment #8)
> > (In reply to Mike Hommey [:glandium] from comment #7)
> > > (In reply to Ting-Yuan Huang from comment #6)
> > > > I tried a slightly different approach to prelink. The dynamic linker was
> > > > changed to cache relocations which can be shared among processes. In
> > > > contrast to prelink, ASLR can be retained to some extent. Foreign libraries
> > > > and executables could also be benefited.
> > > 
> > > How do you share the relocated parts of the libs between different processes?
> > 
> > For the ease of debugging, currently I write them to /system/tmp/* (only
> > relocated parts) and mmap() them when needed.
> 
> That's going to be slow. That's definitely nice for prototyping, but not for
> production :)

Ting-Yuan, do you really mean to write() to the file?  An efficient way is to mmap an empty file on the memory storing data there.  The data is not really being wrote to the storage until necessary.

> 
> > I'll later change to ashmem and maybe have a centralized daemon for security. Any ideas?
> 
> That's basically what i'm doing with our custom linker on android. Anyways,
> for immediate gains for v1, prelinking is the way to go.

Prelinking is followed by some weakness.  As I known, b2g and gonk will be updated separately, prelinking implies that the versions of b2g and gonk should be kept in consistent.

Centralized daemon is a little over-engineering in our case.  Since all our processes are forked from a parent process, keeping a fd crossing fork() & exec(), and distributing the fd number through an environ variable is good enough.  We even don't need to keep the link on the filesystem.  We can unlink the entry on the directory immediately after opening the backing file.  So, steps of doing so are

 1. open a temp file and got a fd
 2. mmap the fd
 3. move data to mapped space
 4. open a read only fd for the temp file
 5. unlink the temp file (avoiding others to change it)
 6. put read-only fd number to an environ variable (you can not lift its read-only privilege)

Then, the fd can be shared among parent and children.  Children can mmap the read-only fd given by the environ variable.

Comment 11

[Ting-Yuan Huang]

(In reply to Thinker Li [:sinker] from comment #10)
> Ting-Yuan, do you really mean to write() to the file?  An efficient way is
> to mmap an empty file on the memory storing data there.  The data is not
> really being wrote to the storage until necessary.

No, I mmap() it with MAP_SHARED and PROT_WRITE. OS decides when to write to storage.

Comment 12

[Ting-Yuan Huang]

prelink prevents libraries from being updated separately. Is this a problem to Firefox OS? If so, we have to either check the incompatibility and fallback, or to adopt other solutions like relocation caching or prelink on target.

Even with fallback, prelink may be ineffective most of the time, depending on how the libraries are updated.

Comment 13

[Andrew Overholt [:overholt]]

We're now marking soft blockers as P4/blocking-basecamp=- so I'm changing this bug to be consistent.

Comment 14

[Ting-Yuan Huang]

Attachment: caching relocations in dynamic linker

Attached is a draft implementation. It saves about 1.6MB per plugin-container instance and a few kilobytes for other processes.

Security is ensured by file permissions. Only root can write relocation caches. Unprivileged processes can only read. When they need libraries that are not cached, relocations fall back to private pages.

There is a centralized table keeping track of addresses to which libraries are loaded. This is also writeable only by root. If unprivileged processes need libraries out of the table, the load addresses must be decided locally. They must skip caches that depend on those locally loaded libraries, otherwise the caches and locally loaded libraries might be incompatible. In B2G this rarely happens.

Currently the caches are placed on /dev/. I'll change it to some dedicated tmpfs. If caches are written to back stores such as /data, it behaves much like a prelinker on target, right? :)

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 688202 [details] [diff] [review]
caching relocations in dynamic linker

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

I haven't looked at great depth, but this seems rather fragile. I'd expect race conditions during boot, where a lot of processes will fight for the cache.

::: linker/linker.c
@@ +724,5 @@
> +}
> +
> +static unsigned skip; // Skip if one of the addr of the library is not found.
> +static unsigned rdonly;
> +static volatile socache *schdr; // Only used by the single-threaded dynamic linker.

volatile is probably not what you want everywhere you use it. It already wouldn't guarantee much in the same process, so cross-process, it's even worse.

@@ +737,5 @@
> +{
> +    *lock = 1;
> +}
> +
> +static volatile socache *init_sc_list(const char *name, unsigned *ro)

name is not used.

@@ +744,5 @@
> +    int fd = -1;
> +    void *m;
> +    int firstcome = 0;
> +
> +    umask(022);

Please don't change umask. If you need a guaranteed mode on a file or directory, chmod it.

@@ +895,5 @@
> +    shdr = (Elf32_Shdr *)((char *)ehdr + ehdr->e_shoff);
> +    shstrs = (char *)(ehdr) + ((Elf32_Shdr *)((char *)shdr + ehdr->e_shstrndx * ehdr->e_shentsize))->sh_offset;
> +
> +    for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
> +        if (strcmp(".data.rel.ro", shstrs + shdr->sh_name) == 0) {

There are relocations outside of .data.rel.ro. Most probably, some libs or programs will have text relocations, which can be treated the same way. But even the relocations on the writeable pages might be worth handling this way too, provided you properly map the data MAP_PRIVATE.

@@ +923,5 @@
> +{
> +    cacheinfo ci = {0, 0, 0};
> +    unsigned loaded_addr;
> +    int fd = -1;
> +    /* Since this is a dynamic linker and should be in the very bottom of the call stack. */

That's not necessarily true (think dlopen())

@@ +957,5 @@
> +        fd = open(cache_name, O_RDONLY);
> +        if (fd == -1)
> +            goto bail;
> +        loaded_addr = (unsigned)mmap((void *)si->ci.base, si->ci.size, PROT_READ | PROT_EXEC | PROT_WRITE,
> +                           MAP_FIXED | MAP_PRIVATE, fd, 0);

Using MAP_FIXED is dangerous here. There could be something else mapped there (like memory mapped by the heap allocator). Remove MAP_FIXED and keep the address check, that will be enough.

@@ +1090,5 @@
>      max_vaddr = (max_vaddr + PAGE_SIZE - 1) & ~PAGE_MASK;
>  
>      *total_sz = (max_vaddr - min_vaddr);
> +#ifdef CACHE_RELOCATIONS
> +    req_base = get_cached_addr(fd, name, *total_sz);

You should do that before the prelinked case.

::: core/armelf.xsc
@@ +78,4 @@
>    /* Adjust the address for the data segment.  We want to align at exactly
>       a page boundary to make life easier for apriori. */
>    . = ALIGN(4096);
> +  .data.rel.ro : { *(.data.rel.ro.local* .gnu.linkonce.d.rel.ro.local.*) *(.data.rel.ro* .gnu.linkonce.d.rel.ro.*) . = ALIGN(4096);}

Why do you move .data.rel.ro?

::: Android.mk
@@ +224,4 @@
>  	export B2G_UPDATER="$(B2G_UPDATER)" && \
>  	export B2G_UPDATE_CHANNEL="$(B2G_UPDATE_CHANNEL)" && \
>  	export ARCH_ARM_VFP="$(ARCH_ARM_VFP)" && \
> +	export EXTRA_DSO_LDOPTS="-Wl,-T,/home/tingyuan/work/B2G.unagi/build/core/armelf.xsc" && \

Obviously, that's not going to work except for you :)

Comment 16

[Ting-Yuan Huang]

Thanks for the review! Do you think this is a viable approach? I know this looks dangerous since security and synchronization issues must be handled at runtime. I'd like to make sure I'm on the right way before diving in. It is more flexible than prelinking. Do we need this flexibility?

(In reply to Mike Hommey [:glandium] from comment #15)
> ::: linker/linker.c
> @@ +724,5 @@
> > +}
> > +
> > +static unsigned skip; // Skip if one of the addr of the library is not found.
> > +static unsigned rdonly;
> > +static volatile socache *schdr; // Only used by the single-threaded dynamic linker.
> 
> volatile is probably not what you want everywhere you use it. It already
> wouldn't guarantee much in the same process, so cross-process, it's even
> worse.

I'll change to a more rigorous way if we really want this.

> There are relocations outside of .data.rel.ro. Most probably, some libs or
> programs will have text relocations, which can be treated the same way. But
> even the relocations on the writeable pages might be worth handling this way
> too, provided you properly map the data MAP_PRIVATE.

But caching the whole file could be a significant overhead. AFAIK the major source of text relocations are non-PIC codes. Since we don't have many executables in B2G and the most important 2 are small, I'm afraid the benefits could be tiny.

> Using MAP_FIXED is dangerous here. There could be something else mapped
> there (like memory mapped by the heap allocator). Remove MAP_FIXED and keep
> the address check, that will be enough.

The MAP_FIXED is copied from the logics that handles prelink. I'm also doubting if this is safe. I tried to remove MAP_FIXED but the kernel seems to always ignore the suggested address.

> @@ +1090,5 @@
> >      max_vaddr = (max_vaddr + PAGE_SIZE - 1) & ~PAGE_MASK;
> >  
> >      *total_sz = (max_vaddr - min_vaddr);
> > +#ifdef CACHE_RELOCATIONS
> > +    req_base = get_cached_addr(fd, name, *total_sz);
> 
> You should do that before the prelinked case.

Android's prelinker strips relocation entries so a prelinked library can't be linked to places other than the designated address. So a prelinked library can't be cached. Did you mean there should be a check to see if it is already prelinked?

> Why do you move .data.rel.ro?

For minimizing private-dirty pages. Originally .data.rel.ro shares pages with .data/.bss on boundaries. This results in a relatively significant waste to small libraries. Compare these layouts:
(. = ALIGN(4096)) 1K .data -- 3K .data.rel.ro -- 1K .bss
(. = ALIGN(4096)) 3K .data.rel.ro -- 1K .data -- 1K .bss

To separate .data.rel.ro and other RW segments on different pages could never be worse. Assume there are X pages (could be fractional) .data.rel.ro and Y pages of other RW segments. The dirty pages when they share pages on boundary are
ceil(X + Y) - floor(X) == ceil((X - floor(X)) + floor(X) + Y) -floor(X)
                       == ceil((X - floor(X)) + Y)
                       >= ceil(Y) (# dirty pages when separated)

Comment 17

[Mike Hommey [:glandium]]

(In reply to Ting-Yuan Huang from comment #16)
> > There are relocations outside of .data.rel.ro. Most probably, some libs or
> > programs will have text relocations, which can be treated the same way. But
> > even the relocations on the writeable pages might be worth handling this way
> > too, provided you properly map the data MAP_PRIVATE.
> 
> But caching the whole file could be a significant overhead. AFAIK the major
> source of text relocations are non-PIC codes. Since we don't have many
> executables in B2G and the most important 2 are small, I'm afraid the
> benefits could be tiny.

I'm not sure about Android/Gonk proper, but simply building with the Android NDK creates text relocations because there are text relocations in the crt objects linked with every executable/library.

What you could do is cache the relocated parts only, that is, create a file with holes, and then map the non-hole parts on based on the relocation offsets.

> > Using MAP_FIXED is dangerous here. There could be something else mapped
> > there (like memory mapped by the heap allocator). Remove MAP_FIXED and keep
> > the address check, that will be enough.
> 
> The MAP_FIXED is copied from the logics that handles prelink. I'm also
> doubting if this is safe. I tried to remove MAP_FIXED but the kernel seems
> to always ignore the suggested address.

The kernel probably doesn't really like that CACHE_BASE is over 0x7fffffff.

> > @@ +1090,5 @@
> > >      max_vaddr = (max_vaddr + PAGE_SIZE - 1) & ~PAGE_MASK;
> > >  
> > >      *total_sz = (max_vaddr - min_vaddr);
> > > +#ifdef CACHE_RELOCATIONS
> > > +    req_base = get_cached_addr(fd, name, *total_sz);
> > 
> > You should do that before the prelinked case.
> 
> Android's prelinker strips relocation entries so a prelinked library can't
> be linked to places other than the designated address. So a prelinked
> library can't be cached.

That's probably fine then.

Comment 18

[Mike Hommey [:glandium]]

With that being said,

(In reply to Ting-Yuan Huang from comment #16)
> Thanks for the review! Do you think this is a viable approach? I know this
> looks dangerous since security and synchronization issues must be handled at
> runtime. I'd like to make sure I'm on the right way before diving in. It is
> more flexible than prelinking. Do we need this flexibility?

Yeah, it is kind of a runtime prelinking. Provided we can figure the concurrency issues, it could be worth, but that really feels risky for a v1.

Comment 19

[Ting-Yuan Huang]

Attachment: some statistics on prelinked libraries

Memory saved by prelink are similar to relocation caches.

Comment 20

[Ting-Yuan Huang]

Attachment: Fix global relocations.

apriori was fixed and augmented to support global prelinking. Here are some notes:

1. elfcopy seems to not support certain libraries. adjust_elf failed on most b2g libraries unless android's linker script are used. I disabled adjust_elf so the file size is the same as the original/not prelinked one.

2. Since bionic's dynamic linker never looks at symbol versions, I disabled the corresponding assertion in apriori.

3. When a symbol is multiply defined, it'll be skipped. The resolution will be deferred to dynamic linker. This is a workaround for LD_PRELOAD.

4. When a symbol can't be found, the resolution will be deferred to dynamic linker.

I don't know at what step the prelinker should be invoked best in the build process. It depends on how we update libraries. Any ideas?

Comment 21

[Justin Lebar (not reading bugmail)]

Comment on attachment 690766 [details] [diff] [review]
Fix global relocations.

This is way beyond my ken.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 690766 [details] [diff] [review]
Fix global relocations.

Hi Ting-Yuan, I don't know this code well enough to review quickly, sorry :(.  mwu may be able to help out.

Comment 23

[Mike Hommey [:glandium]]

Where can the apriori code you've based on for this patch be found? (repo and commit)

Comment 24

[Ting-Yuan Huang]

Hi Justin and Chris, thanks for your response :)

Actually, I'd like to make sure this approach won't pose serious restrictions on how Firefox OS got updated. Prelinker links libraries to fixed locations at compile time according to a predefined address map. This implies the update package would be considerably larger due to address map changes and library dependencies when it comes to partial/incremental update.

Comment 25

[Ting-Yuan Huang]

(In reply to Mike Hommey [:glandium] from comment #23)
> Where can the apriori code you've based on for this patch be found? (repo
> and commit)

Sorry, I forgot this. build/tools/apriori got removed in
b375e71d306f2fd356b9b356b636e568c4581fa1

IIRC, I cloned android 2.3.1. The git head is
16101b8534867f8595025830e5264cd84628f4cb

To build apriori, you also need external/elfcopy
62c1bed1c4505369cac2e72fbe30452a598fb690

and external/elfutils
84cf4183ba6f577ee01abe7f1f5a6d4b23df35a8

Comment 26

[Michael Wu [:mwu]]

Whoa.

Uh, I can probably review this. glandium can too. I'll need to look at this a bit more carefully to understand the risks better.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Ting-Yuan Huang from comment #24)
> Actually, I'd like to make sure this approach won't pose serious
> restrictions on how Firefox OS got updated. Prelinker links libraries to
> fixed locations at compile time according to a predefined address map. This
> implies the update package would be considerably larger due to address map
> changes and library dependencies when it comes to partial/incremental update.

I don't have a good feel for what size difference this would be.  We can test this directly though.  The steps at

https://wiki.mozilla.org/B2G/Updating#Building_updates_for_multiple_software_versions

describe how to build incremental updates.  We can choose a set of revisions from
 - today, day ago, week ago, month ago, 3 months ago
 - make normal builds for those revisions
 - make prelinked builds for those revisions
 - generate incremental updates for the normal builds between "today" and the older versions
 - generate incremental updates for the prelinked builds between "today" and the older versions

Comment 28

[Ting-Yuan Huang]

Sorry, I didn't notice the existence of binary diffs in the update scripts. The overhead is really small since the only differences are in relocated bytes that changed! I built two complete gecko/gaia update MAR in which all libraries are prelinked to different addresses. The incremental update MAR between them is only 387KB. Quite promising.

And I got a problem when trying to get the full FOTA update zip. May I know where can I get the kernel and where to put in vendor/? Quoted from the wiki page:

"The default target in the B2G build system will generate a FOTA update.zip / target files zip when the kernel binary has been copied to the appropriate location under vendor/."

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, that sounds very good :).

The incremental FOTA updates shouldn't be much different than the incremental gecko updates, but marshall can help you get those set up.

Comment 30

[Ting-Yuan Huang]

Attachment: Enable prelink in build system.

Hi Glandium,

Attached patches add apriori and its dependencies to external/ and enable prelink in the build system. Changes to the build system are quite small. Would you please give me some feedback?

# Add apriori, elfcopy and elfutils from Android 2.3.1.
cd /PATH/TO/external
patch -p1 < 1.external.elfutils.84cf4183ba6f577ee01abe7f1f5a6d4b23df35a8.patch
patch -p1 < 2.external.elfcopy.62c1bed1c4505369cac2e72fbe30452a598fb690.patch
patch -p1 < 3.external.apriori.16101b8534867f8595025830e5264cd84628f4cb.patch

# Apply fixes to apriori.
cd /PATH/TO/external
patch -p1 < 4.external.apriori.patch

# Adjust makefiles.
cd /PATH/TO/build
patch -p1 < 5.build.patch
cd /PATH/TO/gonk-misc
patch -p1 < 6.gonk-misc.patch

Comment 31

[Mike Hommey [:glandium]]

The apriori changes look sound, afaict, just a few comments:

+static unsigned int ceilX(unsigned int i, unsigned int x)
+{
+    return (i + x - 1) & ~(x - 1);
+}
+
+static unsigned int floorX(unsigned int i, unsigned int x)
+{
+    return i & ~(x - 1);
+}

These functions assume that x is a power of 2, it should probably be noted somewhere as a comment or an assert().

+#if 0 // Totally wrong.

Oh waw, indeed.

+                min_vaddr = floorX(min_vaddr, 0x1000);
+                max_vaddr = ceilX(max_vaddr, 0x1000);

You probably want to use p_align here, and do that when going through the PT_LOADs instead of after.

+                fsize = ceilX((max_vaddr - min_vaddr) * 2, 0x100000);

Why 0x100000?

+                //fsize = max_vaddr - min_vaddr;

You may want to remove the comment.

This part should definitely be sent upstream because their code only really works by chance.

I can't really comment on the build parts.

Comment 32

[Ting-Yuan Huang]

(In reply to Mike Hommey [:glandium] from comment #31)
> +                min_vaddr = floorX(min_vaddr, 0x1000);
> +                max_vaddr = ceilX(max_vaddr, 0x1000);
> 
> You probably want to use p_align here, and do that when going through the
> PT_LOADs instead of after.

Thanks, I'll explicitly use page alignment here.

The min_vaddr/max_vaddr are for the whole library, among all PT_LOADs (this logic is copied from bionic), so I think it should be fine to align them in the end.

> +                fsize = ceilX((max_vaddr - min_vaddr) * 2, 0x100000);
> 
> Why 0x100000?

Just a number big enough so that we don't have to change the map frequently. I'll add more comments here.

Comment 33

[Ting-Yuan Huang]

Comment on attachment 694793 [details]
Enable prelink in build system.

Hi Michael,

Would you please give me some advices on the modifications to the build system?

Comment 34

[Mike Hommey [:glandium]]

(In reply to Ting-Yuan Huang from comment #32)
> The min_vaddr/max_vaddr are for the whole library, among all PT_LOADs (this
> logic is copied from bionic), so I think it should be fine to align them in
> the end.

Page alignment is not necessarily constant in all PT_LOADs (in theory, although in practice it is), so you'd need to enumerate the segments again. All in all, it's probably simpler (and more robust) to do it all at once.

> > +                fsize = ceilX((max_vaddr - min_vaddr) * 2, 0x100000);
> > 
> > Why 0x100000?
> 
> Just a number big enough so that we don't have to change the map frequently.

It would probably be better as a command line option, then.

Comment 35

[Ting-Yuan Huang]

Comment on attachment 694793 [details]
Enable prelink in build system.

Hi Kyle, would you please give me some advices on the modification to Makefiles?

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 694793 [details]
Enable prelink in build system.

The build system changes here look reasonable.  cjones/dhylands or someone more familiar should probably sign off on actually landing it though.

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

New repository pushed from aosp/gingerbread:

https://github.com/mozilla-b2g/platform_external_elfcopy
https://github.com/mozilla-b2g/platform_external_elfutils

And please make your patches pull requests to b2g-manifest and platform_build.

Comment 38

[Ting-Yuan Huang]

Attachment: link to patch on github: apriori

Comment 39

[Ting-Yuan Huang]

Attachment: link to patch on github: build/core/binary.mk

Comment 40

[Ting-Yuan Huang]

Attachment: link to patch on github: gonk-misc

Comment 41

[Ting-Yuan Huang]

Attachment: link to patch on github: manifests

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

dhylands, do you feel comfortable enough with the gonk/b2g build system to do the review here?

Comment 43

[Dave Hylands [:dhylands]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #42)
> dhylands, do you feel comfortable enough with the gonk/b2g build system to
> do the review here?

sure. If I have any questions, I know who to talk to :)

Comment 44

[Dave Hylands [:dhylands]]

Comment on attachment 700332 [details]
link to patch on github: build/core/binary.mk

Nothing jumnps out at me as being wrong (I'm assuming that this has been tested)

Comment 45

[Dave Hylands [:dhylands]]

Comment on attachment 700334 [details]
link to patch on github: gonk-misc

Nothing jumnps out at me as being wrong (I'm assuming that this has been tested)

Comment 46

[Dave Hylands [:dhylands]]

I don't really have any way of knowing if the manifests are right. so I'm going to punt on reviewing that.

Comment 47

[Justin Lebar (not reading bugmail)]

Comment on attachment 700335 [details]
link to patch on github: manifests

This switches a number of manifests from releases-mozilla-central/master to git.mozilla.org/releases/gecko.git/gecko-18.  It also switches Gaia repositories.

Setting aside the question of whether these changes are a good idea, do they have to be done in this bug?  If we reduce the size of the build-system change to the minimum required to fix this bug, I bet cjones (or whoever) would have an easier time reviewing.

Comment 48

[Justin Lebar (not reading bugmail)]

> This switches a number of manifests from releases-mozilla-central/master to 
> git.mozilla.org/releases/gecko.git/gecko-18.  It also switches Gaia repositories.

Oh, I see; the pull request is showing too many changes, for some reason.  Ting-Yuan is going to try to rebase and see if that fixes it...

Comment 49

[Ting-Yuan Huang]

Attachment: link to patch on github: manifests

Sorry, the base I'd been working on is too old.

Comment 50

[Ting-Yuan Huang]

In this patch/pull-request I need to bring 3 projects back from gingerbread:

1. build/tools/apriori
2. external/elfcopy
3. external/elfutils

and modify contents under the following:

4. build/
5. gonk-misc

and update manifests accordingly:

6. .repo/manifests/

Are there any suggest ways to do so?

Currently I put those changes on github and request pulls from github.com/mozilla-b2g, but some of the manifests, such as .repo/manifests/otoro.xml doesn't have github.com/mozilla-b2g; It has "https://git.mozilla.org/b2g" instead.

Comment 51

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 700997 [details]
link to patch on github: manifests

Looks like jlebar is reviewing.

Comment 52

[Justin Lebar (not reading bugmail)]

I'm happy if you leave the repositories on github and point all the manifests to github for these new repositories.  Otherwise we have to involve releng to create the new repositories, and they seem to be short on manpower at the moment.

cjones, is that OK with you?

Comment 53

[Ting-Yuan Huang]

(In reply to Justin Lebar [:jlebar] from comment #52)
> I'm happy if you leave the repositories on github and point all the
> manifests to github for these new repositories.  Otherwise we have to
> involve releng to create the new repositories, and they seem to be short on
> manpower at the moment.
> 
> cjones, is that OK with you?

The newly created projects would be fine but the existing ones are not. The changes to existing projects get only into github. How can I pull-request them? (into git.mozilla.org) or they are synchronized automatically and regularly?

Comment 54

[Ting-Yuan Huang]

Attachment: link to patch on github: manifests

1. newly created project are pointed to github.com/mozilla-b2g
2. changes to existing projects are automatically pulled from github.com/mozilla-b2g.

Comment 55

[Justin Lebar (not reading bugmail)]

Comment on attachment 701087 [details]
link to patch on github: manifests

r=me, but could you please squash this into one commit when you land?

Comment 56

[Ting-Yuan Huang]

(In reply to Justin Lebar [:jlebar] from comment #55)
> Comment on attachment 701087 [details]
> link to patch on github: manifests
> 
> r=me, but could you please squash this into one commit when you land?

Done.

For minimizing the risks, prelink is disabled by default. What product should we enable? Or when should we enable it on all products?

Like other build options, it can be enabled by an environment variable:
ENABLE_GLOBAL_PRELINK=1

Comment 57

[Mike Hommey [:glandium]]

Comment on attachment 700329 [details]
link to patch on github: apriori

preserve_ratio and preserve_align don't seem quite descriptive names of what they do, and their descriptions in cmdline.c are kind of cryptic.

I'd also suggest to remove the #if 0'ed parts, so that if there are changes in upstream apriori, they don't go unnoticed when merging.

Comment 58

[Ting-Yuan Huang]

(In reply to Mike Hommey [:glandium] from comment #57)
> Comment on attachment 700329 [details]
> link to patch on github: apriori
> 
> preserve_ratio and preserve_align don't seem quite descriptive names of what
> they do, and their descriptions in cmdline.c are kind of cryptic.

I've changed to:
alloc-ratio: ratios of allocated space to loaded sizes of libraries
alloc-alignment: alignments of allocated space for libraries

> 
> I'd also suggest to remove the #if 0'ed parts, so that if there are changes
> in upstream apriori, they don't go unnoticed when merging.

done.

Comment 59

[Shian-Yow Wu [:swu]]

Pull requests merged.

https://github.com/mozilla-b2g/platform_external_apriori/pull/1
https://github.com/mozilla-b2g/platform_build/pull/12
https://github.com/mozilla-b2g/gonk-misc/pull/66
https://github.com/mozilla-b2g/b2g-manifest/pull/43

Comment 60

[Andreas Gal :gal]

Guys, we are in the final stabilization phase and this is not a blocker. If this causes the slightest problems back it out right away. Similar work should land on the post 1.0 branch, which should come alive tomorrow.

Comment 61

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Andreas Gal :gal from comment #60)
> Guys, we are in the final stabilization phase and this is not
> a blocker. If this causes the slightest problems back it out
> right away. Similar work should land on the post 1.0 branch,
> which should come alive tomorrow.

Ting-Yuan had ensured the merges won't cause any changes but adding new host-only tools by default. However, I've still helped revert commits in build, gonk-misc and manifest. Let's re-pull them tomorrow.

Comment 62

[Michael Vines [:m1] [:evilmachines]]

Post 1.0 branch is still not alive for gonk-misc, and this landed again.  Can you please coordinate with the moz release engineering folks on this?

Comment 63

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Shian-Yow Wu from comment #59)
> Pull requests merged.
> 
> https://github.com/mozilla-b2g/platform_external_apriori/pull/1
> https://github.com/mozilla-b2g/platform_build/pull/12
> https://github.com/mozilla-b2g/gonk-misc/pull/66
> https://github.com/mozilla-b2g/b2g-manifest/pull/43

These PRs make the build crash for previous declaration of 'strlen' function on osx Mountain Lion.

./external/elfutils/config-compat-darwin.h:41:22: error: static declaration of 'strnlen' follows non-static declaration
/usr/include/string.h:143:8: note: previous declaration of 'strnlen' was here

Comment 64

[Ting-Yuan Huang]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #63)
> (In reply to Shian-Yow Wu from comment #59)
> > Pull requests merged.
> > 
> > https://github.com/mozilla-b2g/platform_external_apriori/pull/1
> > https://github.com/mozilla-b2g/platform_build/pull/12
> > https://github.com/mozilla-b2g/gonk-misc/pull/66
> > https://github.com/mozilla-b2g/b2g-manifest/pull/43
> 
> These PRs make the build crash for previous declaration of 'strlen' function
> on osx Mountain Lion.
> 
> ./external/elfutils/config-compat-darwin.h:41:22: error: static declaration
> of 'strnlen' follows non-static declaration
> /usr/include/string.h:143:8: note: previous declaration of 'strnlen' was here

By default prelink is disabled and apriori/elfcopy/elfutils should not be built. Did you enabled ENABLE_GLOBAL_PRELINK? If so, can we fix it without back it out?

Comment 65

[Shian-Yow Wu [:swu]]

Hi Alex,

Would you please comment on when post 1.0 branch for gonk-misc will be available?  Thank you.

Comment 66

[Ting-Yuan Huang]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #63)
> (In reply to Shian-Yow Wu from comment #59)
> > Pull requests merged.
> > 
> > https://github.com/mozilla-b2g/platform_external_apriori/pull/1
> > https://github.com/mozilla-b2g/platform_build/pull/12
> > https://github.com/mozilla-b2g/gonk-misc/pull/66
> > https://github.com/mozilla-b2g/b2g-manifest/pull/43
> 
> These PRs make the build crash for previous declaration of 'strlen' function
> on osx Mountain Lion.
> 
> ./external/elfutils/config-compat-darwin.h:41:22: error: static declaration
> of 'strnlen' follows non-static declaration
> /usr/include/string.h:143:8: note: previous declaration of 'strnlen' was here

Fixed in
https://github.com/mozilla-b2g/platform_external_elfutils/commit/72940dec691fa3255e13df01f8c53b620e446066

and tested on an osx10.8 machine.

Comment 67

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is a big memory win that will let us keep ~one more app running in the background and boost memory allocatable by apps by ~4MB.  Need to get in the enable pipeline asap.

Comment 68

[Dietrich Ayala (:dietrich)]

Blocking+. If this isn't ready by 1.1 freeze, can re-evaluate blocking status based on memory usage stats at that time.

Comment 69

[Ting-Yuan Huang]

Those changes are landed to master branches in mozilla-b2g on github. Specifically:
https://github.com/mozilla-b2g/platform_external_apriori
https://github.com/mozilla-b2g/platform_build
https://github.com/mozilla-b2g/gonk-misc
https://github.com/mozilla-b2g/b2g-manifest

So I don't have to land them to v1-train, right?

Comment 70

[Joe Cheng [:jcheng] (please needinfo)]

(In reply to Ting-Yuan Huang from comment #69)
> Those changes are landed to master branches in mozilla-b2g on github.
> Specifically:
> https://github.com/mozilla-b2g/platform_external_apriori
> https://github.com/mozilla-b2g/platform_build
> https://github.com/mozilla-b2g/gonk-misc
> https://github.com/mozilla-b2g/b2g-manifest
> 
> So I don't have to land them to v1-train, right?

it seems to be build related. not sure if releng has further comments? thanks

Comment 71

[Chris AtLee [:catlee]]

It looks like the v1-train manifests pull from the v1-train branch for all of the repositories you mentioned. So I think yes, they should be landed on v1-train. Perhaps jhford knows for sure?

Comment 72

[Michael Wu [:mwu]]

These repos should *not* be landed to v1-train. This feature was not approved for v1.

Comment 73

[James Ho]

This is marked leo+ and has been ready for 2 months. This is not really a product feature but a memory performance enhancement. Should we re-nominate it?

Comment 74

[Ting-Yuan Huang]

After discussing with our QA, Al Tsai, I'm going to enable prelink on all ARM devices(at least on the master branch). Is it fine to enable it in the following? (all but emulator-x86)

- galaxy-s2
- galaxy-nexus
- nexus-s
- nexus-s-4g
- otoro
- unagi
- inari
- keon
- leo
- hamachi
- tara
- pandaboard
- emulator

Comment 75

[Michael Vines [:m1] [:evilmachines]]

(In reply to James Ho from comment #73)
> This is marked leo+ and has been ready for 2 months. This is not really a
> product feature but a memory performance enhancement. Should we re-nominate
> it?

Uplifting to v1.1 sgtm, we have some work on our side to port this in the commercial builds once it lands.  Unfortunately v1.0.1 is where we need this the most but that's not really on the table right now.

Comment 76

[Ting-Yuan Huang]

Attachment: link to patch on github: apriori

Hi Glandium,

The changes made apriori more fault tolerant-able. Could you please review it?

Comment 77

[Ting-Yuan Huang]

Attachment: link to patch on github: config.sh, setup.sh gonk-misc/prelink.map

Hi Dave,

I'm going to enable prelink on the following devices in the default configurations:
otoro, unagi, keon, inari, leo, hamachi

Could you please review these 2 pull requests?

Comment 78

[Dave Hylands [:dhylands]]

Has this been tested with debug builds as well? Or perhaps I should ask, what happens when something doesn't fit into the size allocated in the prelink map?

Comment 79

[Ting-Yuan Huang]

(In reply to Dave Hylands [:dhylands] from comment #78)
> Has this been tested with debug builds as well? Or perhaps I should ask,
> what happens when something doesn't fit into the size allocated in the
> prelink map?

Thanks for reminding, I've not tested with debug builds yet. :(

If the size doesn't fit, the build would fail at the very last steps and adjustments are needed. For minimizing the probability of failure, currently the map is generated by 4 times of the required size and up aligned to 2MB per library.

The alternative/additional guard is to skip libraries that don't fit and just give a warning. Instead of forcing human intervention, is this better?

Comment 80

[Dave Hylands [:dhylands]]

(In reply to Ting-Yuan Huang from comment #79)
> (In reply to Dave Hylands [:dhylands] from comment #78)
> > Has this been tested with debug builds as well? Or perhaps I should ask,
> > what happens when something doesn't fit into the size allocated in the
> > prelink map?
> 
> Thanks for reminding, I've not tested with debug builds yet. :(
> 
> If the size doesn't fit, the build would fail at the very last steps and
> adjustments are needed. For minimizing the probability of failure, currently
> the map is generated by 4 times of the required size and up aligned to 2MB
> per library.
> 
> The alternative/additional guard is to skip libraries that don't fit and
> just give a warning. Instead of forcing human intervention, is this better?

In any case, if it fails, we need a really clear error message so that the developer understands whats going on. I'm guessing that most developers won't be familiar with the prelinking stuff.

I think that for eng builds we should just produce a warning, but produce a build that works, even if it isn't quite as fast.

For non-eng builds, I think its ok to require the prelink map to be correct.

Comment 81

[Ting-Yuan Huang]

I updated the pull request to apriori to skip libraries that don't fit and fixed two bugs by the way. So all the changes are:

* apriori
  * Reserve more space to libraries by default.
  * Allow libraries involved to be absent from the link map.
  * Skip libraries that don't fit in prelink.map.
  * Fixed two bugs.
* Update prelink.map
* Enable prelink on otoro, unagi, keon, inari, leo, hamachi by default.

I've played for a while for several configurations of prelink.map that are too small, incomplete, or both. It looks fine and there's no extra errors or fails in the logs when compared to the not-prelinked builds. I also tested the debug builds. Looks fine, too.

Dave, are those changes safe?

Glandium, there are two more commits. Would you please also review them?

Comment 82

[Mike Hommey [:glandium]]

Comment on attachment 736723 [details]
link to patch on github: apriori

reviewed 7821f66, 8e201c6, ab9dec1, and 5fa2ae8

Comment 83

[Alex Keybl [:akeybl]]

(In reply to James Ho from comment #73)
> This is marked leo+ and has been ready for 2 months. This is not really a
> product feature but a memory performance enhancement. Should we re-nominate
> it?

Let's move back to leo? so that this can be reviewed in triage. Likely not a blocker at this point.

Comment 84

[Ting-Yuan Huang]

Alex, should we land it right now? or until leo+?

Comment 85

[Alex Keybl [:akeybl]]

Feel free to land to trunk, that only requires r+. Please ask for approval if we think this is desirable for mozilla-b2g18.

Comment 86

[Michael Wu [:mwu]]

Can you file a follow up to move the enable/disable flag out of config.sh? That's an inappropriate place to place device specific settings. Those things should be placed in device directories or simply enabled globally.

Comment 87

[Michael Wu [:mwu]]

The change to the mozilla-b2g/B2G repo was reverted. It unconditionally enabled prelinking on all branches, including ones where prelinking wasn't fully landed (or approved), so a standard ./config.sh unagi build was broken. On v1-train, the unagi manifest doesn't have apriori but gonk-misc has prelinking landed there, so things fail.

Comment 88

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

I want to test your patch on FFOS 1.2 .

Could you please rebase your patch to most recent FFOS.

Comment 89

[Andreas Gal :gal]

Lets reland this. This seems important.

Comment 90

[Ting-Yuan Huang]

Thinker, will Nuwa (bug 771765) be landed in FFOS 1.2? Because the memory and computing efforts saved by this bug is a proper subset of Nuwa, should we land this bug?

Comment 91

[Andreas Gal :gal]

Nuwa will definitely not land for 1.2, so if this is low risk, we can consider taking it for 1.2. In the alternative both or just this or just nuwa can land for 1.3.

Comment 92

[Ting-Yuan Huang]

Marionette and mochitest passed on emulator after enabling prelink. I think this should be low risk for 1.2. I'll update the PRs after making sure they won't break builds of other branches.

Comment 93

[Ting-Yuan Huang]

Attachment: adjust default allocation sizes

The larger values is for creating/allocating the map only; When calculating the size required by a library, we need them to be as small as possible.

Comment 94

[Ting-Yuan Huang]

Attachment: adjust default allocation sizes

Sorry, mistake in the attachment.

Comment 95

[Ting-Yuan Huang]

Attachment: revert prelink on v1.0.*

Reverting prelink on v1.0.*

Comment 96

[Ting-Yuan Huang]

Attachment: Update prelink.map accroding to emulator's libraries

Update prelink.map according to emulator's libraries. Previously it's generated for unagi and some manufacturer specific libraries are listed.

Comment 97

[Ting-Yuan Huang]

Attachment: Enable prelink on some devices by default

Enabling prelink on the following devices by default:
otoro, unagi, keon, inari, leo, hamachi, peak, helix, wasabi
emulator, emulator-jb

This PR depends on:
https://github.com/mozilla-b2g/gonk-misc/pull/118
https://github.com/mozilla-b2g/gonk-misc/pull/119
https://github.com/mozilla-b2g/gonk-misc/pull/120
https://github.com/mozilla-b2g/gonk-misc/pull/121
https://github.com/mozilla-b2g/gonk-misc/pull/122
https://github.com/mozilla-b2g/platform_external_apriori/pull/3
https://github.com/mozilla-b2g/platform_external_apriori/pull/4

Comment 98

[Ting-Yuan Huang]

Hi Tapas,

Please find the following PRs (before they are landed) for testing, thanks.
https://github.com/mozilla-b2g/gonk-misc/pull/121
https://github.com/mozilla-b2g/platform_external_apriori/pull/3
https://github.com/mozilla-b2g/B2G/pull/284

You'll have to re-run config.sh or add ENABLE_GLOBAL_PRELINK=1 to .config manually.

Comment 99

[Mandyam Vikram]

What would be the risk level of bringing this in 1.2?
Will global pre-linking be turned off by default?

Comment 100

[Andreas Gal :gal]

Mandyam, this is your call. Since we are past FC you guys can mark anything you need and we will uplift. I don't think the risk is huge here.

Comment 101

[Mandyam Vikram]

We'll check it out first.

Comment 102

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

I am seeing a big improvement in Preallocate process USS after applying patch from #comment 98

Previously it was like:

PSS         USS
16046	11548

Now it is

PSS         USS
10808K    8808K

Thanks for your patch

Comment 103

[Andreas Gal :gal]

Marking as a regression from 1.1 since 1.2 uses more memory

Comment 104

[Andreas Gal :gal]

Can you please land on branch?

Comment 105

[Andreas Gal :gal]

How are you measuring these numbers? I would like an ongoing graph for this.

Comment 106

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

@Andreas,

You need to parse |adb shell procrank| output using some script.

Comment 107

[Michael Vines [:m1] [:evilmachines]]

|b2g-procrank| will give this info straight up.

Comment 108

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Hi Ting-Yuan Huang ,

I guess that we can enable/disable during build by ENABLE_GLOBAL_PRELINK . 

But I also see that you changed prelink.map in gonk-misc . 

Can you please let me know how to disbale this fix in build time. It may help us in future after landing this fix.

Comment 109

[Ting-Yuan Huang]

Hi Tapas,

Please set ENABLE_GLOBAL_PRELINK to 0 or remove it from .config. Alternatively, You may remove the export entirely, i.e., don't apply [1], it's a 3-line-diff patch exporting ENABLE_GLOBAL_PRELINK=1.

prelink.map is changed because it was previously generated for unagi and some manufacturer libraries are listed. Now it's for emulator and only contains most common libraries. Other devices will work, although the libraries not listed won't be prelinked. The number of references from libxul.so to those libraries are very limited so excluding them impact the effectiveness very little.

The third patch fixes size estimation in prelinker. Without it the prelinker still works, but will become less and less effective when the sizes of libraries grow.

[1] https://github.com/mozilla-b2g/B2G/pull/284

Comment 110

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Hi Ting-Yuan Huang,

I tried by cloning platform_external_apriori and platform_external_elfcopy from your github workspace https://github.com/ting-yuan

Github is showing that you forked this from following places: 

[1] https://github.com/mozilla-b2g/platform_external_apriori
[2] https://github.com/mozilla-b2g/platform_external_elfcopy

So, If I simply clone above [1][2] repo and apply your fixes from #comment 98 then I should be fine. Please confirm me.

Comment 111

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Can anyone tell me where these repo are coming from ? 

[1] https://github.com/mozilla-b2g/platform_external_apriori
[2] https://github.com/mozilla-b2g/platform_external_elfcopy

I am trying to track original repo. It will help us to maintain it in long run.

Comment 112

[Ting-Yuan Huang]

> So, If I simply clone above [1][2] repo and apply your fixes from #comment
> 98 then I should be fine. Please confirm me.

Pull requests in #comment 98 are asking for merges from HEAD of the master branch, so the fix is already applied in [1].

> Can anyone tell me where these repo are coming from ? 

apriori was originally part of build/ in android and got removed after gingerbread.
https://android.googlesource.com/platform/build/+/gingerbread/tools/apriori/

elfcopy and elfutils
https://android.googlesource.com/platform/external/elfcopy/
https://android.googlesource.com/platform/external/elfutils/

Comment 113

[Mandyam Vikram]

We should try to land this for v1.2. he memory savings are worthwhile.

Comment 114

[Mandyam Vikram]

Please hold off on landing this for a bit until we're ready for it.

Comment 115

[Michael Vines [:m1] [:evilmachines]]

Why wait?  It's disabled by default.

Comment 116

[Mandyam Vikram]

Sorry for the confusion. Please go ahead and land the patch.

Comment 117

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Ting-Yuan Huang from comment #112)

Hey your patch works perfectly for 7627a QRD FFOS 1.2 . Can you please also rebase your patch for jb port of FFOS 1.3 ?

Comment 118

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

I guess that we need a different patchset for FFOS 1.2 and jellybean port of FFOS 1.3 .

Comment 119

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Any update for 1.3

Comment 120

[Ting-Yuan Huang]

May I know where the jellybean port of FFOS 1.3 is? I can't find branch 1.3 in gonk-misc, b2g-manifest, etc.

Comment 121

[Michael Vines [:m1] [:evilmachines]]

N4 is running JB_MR2 -- https://github.com/mozilla-b2g/b2g-manifest/blob/master/nexus-4.xml

Comment 122

[Ting-Yuan Huang]

jellybean's elfutils changes a lot. Specifically, libebl_arm is removed altogether. Unfortunately apriori needs libebl_arm. I need some time to resolve this. For evaluation and testing, I can quickly provide a patch that puts apriori into prebuilts. Will this work for you?

Comment 123

[Ting-Yuan Huang]

JB also removed supports to prelinking in bionic/linker, so fixing the apriori build or simply supplying apriori in prebuilts is not enough. I can fix this but need a couple of days maybe. Maintaining patches to bionic is also inevitable. Do we really need this on JB?

Comment 124

[Mike Hommey [:glandium]]

(In reply to Ting-Yuan Huang from comment #123)
> JB also removed supports to prelinking in bionic/linker

And it probably has a good reason for that. Maybe we should do the same.

Comment 125

[Thinker Li [:sinker]]

We don't need to take care jellybean, since this patch is only for 1.2.  For 1.3, Nuwa is already landed, disabled by default for now.  We expect that Nuwa is enabled at 1.3, so this patch do no much after that.

Comment 126

[Mandyam Vikram]

Can we land the patch for 1.2 now? Thanks.h

Comment 127

[Michael Wu [:mwu]]

Comment on attachment 811839 [details]
revert prelink on v1.0.*

I think it's too late to be concerned about 1.0.x and 1.1. Assuming prelink is disabled by default there, we should be ok with just leaving things as is there.

Comment 128

[Michael Wu [:mwu]]

Comment on attachment 811842 [details]
Update prelink.map accroding to emulator's libraries

Rubberstamp. I'll assume this is a reasonable and tested update of the mapping.

Comment 129

[Michael Wu [:mwu]]

(In reply to Mandyam Vikram from comment #126)
> Can we land the patch for 1.2 now? Thanks.h

The necessary parts have all landed, though the mapping may need to be updated.

What remains is to enable prelinking, which should be done on the CAF side.

(In reply to Thinker Li [:sinker] from comment #125)
> We don't need to take care jellybean, since this patch is only for 1.2.  For
> 1.3, Nuwa is already landed, disabled by default for now.  We expect that
> Nuwa is enabled at 1.3, so this patch do no much after that.

Sounds great. We can back out prelink then, I assume.

(In reply to Mike Hommey [:glandium] from comment #124)
> (In reply to Ting-Yuan Huang from comment #123)
> > JB also removed supports to prelinking in bionic/linker
> 
> And it probably has a good reason for that. Maybe we should do the same.

Assuming you're hinting at ASLR, agreed. I think Nuwa will get us most of the benefits of prelinking without losing too much ASLR. This assumes the RNG is reasonably seeded by the time we start, which is probably a terrible assumption. Maybe we need to add a way for phones to save/restore entropy if there isn't a way already.

Comment 130

[Ting-Yuan Huang]

(In reply to Michael Wu [:mwu] from comment #127)
> Comment on attachment 811839 [details]
> revert prelink on v1.0.*
> 
> I think it's too late to be concerned about 1.0.x and 1.1. Assuming prelink
> is disabled by default there, we should be ok with just leaving things as is
> there.

There seems to be no better way to enable prelink in per device basis than in config.sh and setup.sh; Both of them affect all branches. Because 1.0.x is not backed out cleanly, changes in config.sh and setup.sh will break 1.0.x build. Or we don't care about 1.0.x anymore, do we?

Comment 131

[Ting-Yuan Huang]

> What remains is to enable prelinking, which should be done on the CAF side.

I see. If we are not going to enable it in Mozilla's reference implementations, please forget my last comment.

Comment 132

[Shian-Yow Wu [:swu]]

Landed to branch 1.2 and master, with disabled by default.

https://github.com/mozilla-b2g/platform_external_apriori/pull/3
https://github.com/mozilla-b2g/platform_external_apriori/pull/4
https://github.com/mozilla-b2g/gonk-misc/pull/121
https://github.com/mozilla-b2g/gonk-misc/pull/122

Comment 133

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Thinker Li [:sinker] from comment #125)
> We don't need to take care jellybean, since this patch is only for 1.2.  For
> 1.3, Nuwa is already landed, disabled by default for now.  We expect that
> Nuwa is enabled at 1.3, so this patch do no much after that.

NUWA (bugid 771765) patches has already landed in 1.3 . In my build, I enabled NUWA by following https://bugzilla.mozilla.org/show_bug.cgi?id=771765#c213 . But I don't see any significant improvement in USS of preallocated process as I see for this patch.

Can you please suggest something which can take care jellybean

Comment 134

[Michael Wu [:mwu]]

Measuring NUWA is more difficult. The improvement is more visible when you open more apps. Measuring the preallocated process won't show up, but NUWA should save more memory than prelink when multiple apps are loaded.

Comment 135

[Mike Lee [:mlee]]

Setting Target Milestone for all FxOS Perf 1.2 issues fixed for 10.25.

Comment 136

[Ting-Yuan Huang]

> Can you please suggest something which can take care jellybean

A small patch to bionic would work. Although building apriori in JB code base requires nontrivial efforts, supplying it as binary in prebuilts is easy.

Assuming we are going to use NUWA, the main purpose of this reply is to clear needinfo.