Last Comment Bug 686805 - Do incremental zip decompression in our linker
: Do incremental zip decompression in our linker
Status: RESOLVED FIXED
[Snappy:p1]
: feature, perf
Product: Core
Classification: Components
Component: mozglue (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla13
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 686801 683127 717540 720704 721321 729596
Blocks: 674579 729604 748488 729641
  Show dependency treegraph
 
Reported: 2011-09-14 16:40 PDT by (dormant account)
Modified: 2013-12-27 14:34 PST (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds concurrent decompression of seekable-compressed libraries (WIP) (75.77 KB, patch)
2012-01-05 04:16 PST, Julian Seward [:jseward]
no flags Details | Diff | Review
Program to compress files into seekable compressed streams (5.75 KB, text/plain)
2012-01-05 04:22 PST, Julian Seward [:jseward]
no flags Details
Disable SEGV handler and fork() on desktop (apply first) (1.75 KB, patch)
2012-01-09 04:45 PST, Julian Seward [:jseward]
no flags Details | Diff | Review
Rebase against bug 683127 patches of 6 Jan (apply second) (56.27 KB, patch)
2012-01-09 04:48 PST, Julian Seward [:jseward]
no flags Details | Diff | Review
Program to compress files into seekable compressed streams (7.20 KB, patch)
2012-01-18 06:58 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 1 - Make Mappable::munmap, Mappable1stPagePtr::munmap and MappedPtr::munmap private (2.44 KB, patch)
2012-01-25 02:30 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space (10.99 KB, patch)
2012-01-25 02:33 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 1 - Make Mappable::munmap, Mappable1stPagePtr::munmap and MappedPtr::munmap private (3.15 KB, patch)
2012-01-25 06:34 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space (11.57 KB, patch)
2012-01-25 06:46 PST, Mike Hommey [:glandium]
taras.mozilla: review+
jseward: review+
Details | Diff | Review
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space (11.65 KB, patch)
2012-01-26 06:46 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
WIP part 3 & 4 (43.59 KB, patch)
2012-01-26 11:23 PST, Mike Hommey [:glandium]
jseward: feedback+
Details | Diff | Review
part 3 - Tool to generate seekable compressed streams (9.94 KB, patch)
2012-02-09 08:06 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 3 - Tool to generate seekable compressed streams (9.62 KB, patch)
2012-02-09 08:08 PST, Mike Hommey [:glandium]
jseward: review+
taras.mozilla: review-
Details | Diff | Review
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space (11.60 KB, patch)
2012-02-15 01:32 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 3 - Tool to generate seekable compressed streams (9.38 KB, patch)
2012-02-15 01:35 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 4 - Make the linker load libraries with on-demand decompression when they are seekable compressed streams (21.80 KB, patch)
2012-02-15 01:41 PST, Mike Hommey [:glandium]
jseward: review+
taras.mozilla: review+
Details | Diff | Review
part 5 - Optionally make libxul.so a seekable compressed stream on Android (3.91 KB, patch)
2012-02-15 02:30 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 6 - Add functions to display stats about seekable compressed streams (9.17 KB, patch)
2012-02-15 06:54 PST, Mike Hommey [:glandium]
no flags Details | Diff | Review
part 6 - Add functions to display stats about seekable compressed streams (9.17 KB, patch)
2012-02-15 06:55 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 7 - Hook linker stats in Startup Timeline (4.09 KB, patch)
2012-02-15 06:56 PST, Mike Hommey [:glandium]
taras.mozilla: review+
khuey: review+
Details | Diff | Review
part 4 - Make the linker load libraries with on-demand decompression when they are seekable compressed streams (22.20 KB, patch)
2012-02-16 04:09 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Review
part 6 - Add functions to display stats about seekable compressed streams (9.14 KB, patch)
2012-02-16 04:10 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 5 - Optionally make libxul.so a seekable compressed stream on Android (3.90 KB, patch)
2012-02-16 07:27 PST, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Review
part 5 - Optionally make libxul.so a seekable compressed stream on Android (3.92 KB, patch)
2012-02-16 07:34 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review
part 7 - Display linker stats on each Startup Timeline event (4.10 KB, patch)
2012-02-16 08:53 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Review

Description (dormant account) 2011-09-14 16:40:24 PDT
In order to improve startup responsiveness we should decompress libxul incrementally(via segfault handler)
Comment 1 Julian Seward [:jseward] 2012-01-05 04:16:42 PST
Created attachment 586035 [details] [diff] [review]
Adds concurrent decompression of seekable-compressed libraries (WIP)

Taras, yell if this is the wrong place for this patch.

Adds concurrent segfault based library decompression to Mike Hommey's
new runtime linker patch in bug 683127.  Is very much a WIP and is 
based on an older version of Mike's linker (27 Dec).
Comment 2 Julian Seward [:jseward] 2012-01-05 04:22:10 PST
Created attachment 586037 [details]
Program to compress files into seekable compressed streams

A program to generate seekable compressed streams for use by the
linker mod in previous patch.  Not integrated into the build system.
Uses a block size of 16KB at the moment.
Comment 3 Julian Seward [:jseward] 2012-01-05 04:40:00 PST
(In reply to Julian Seward from comment #1)
> Created attachment 586035 [details] [diff] [review]
> Adds concurrent decompression of seekable-compressed libraries (WIP)

Some additional comments:

For each library in seekable-compressed-stream (SZS) format, it creates a
child thread to decompress the library at the time it is dlopened.  The
child thread decompresses the library in ascending order of address space
(ie, sequentially), but cooperates with a segfault handler to detect cases
where firefox (or the linker) accesses pages of the library that have not
yet been decompressed.  In such cases it gives priority to these out-of-order
blocks and decompresses them immediately, so as to service the fault as
quickly as possible.  Then goes back to sequential decompression.

This succeeds in loading and starting Firefox on x86_64-linux.

It is though pretty hairy:

* if anybody else installs a segfault handler, we are hosed: segfaults
  resulting from accesses to not-yet-decompressed pages are not handled
  by the decompress mechanism, and generally cause Firefox to exit early.

* if Firefox forks, we are hosed: only the thread that calls fork exists
  in the child process.  In particular, all the decompress threads are missing,
  which means any segfaults that happen in the child can never lead to page
  decompression, and the child hangs, sometimes leading to the parent hanging too.

* there may be other unknown instability; I am unclear how reliable this is.

In addition, it seriously messes with our toolery:

* because the libraries are not loaded in a "standard" way, GDB and
  Valgrind do not generate sane stack traces as they cannot find the
  relevant debuginfo objects.  This makes debugging difficult.

* for reasons unknown to me, even though this appears to work OK on the
  command line, when run under GDB it segfaults.

* it breaks Valgrind (it is unusable) since V's JIT takes segfaults
  when jitting application code on as-yet decompressed pages, and V has as yet
  no way to route such segfaults back to the appropriate handler.  This means
  it is impossible to check for memory corruption (V/Memcheck) or exit-time
  races (V/Helgrind) right now.
Comment 4 Julian Seward [:jseward] 2012-01-09 04:45:02 PST
Created attachment 586959 [details] [diff] [review]
Disable SEGV handler and fork() on desktop (apply first)

This is a kludge to make it work on desktop Linux.  A better solution w.r.t.
the conflict between segfault handlers is needed.
Comment 5 Julian Seward [:jseward] 2012-01-09 04:48:08 PST
Created attachment 586961 [details] [diff] [review]
Rebase against bug 683127 patches of 6 Jan (apply second)
Comment 6 Mike Hommey [:glandium] 2012-01-18 06:58:35 PST
Created attachment 589491 [details] [diff] [review]
Program to compress files into seekable compressed streams

This integrates the program in the build system, and C++ize it a bit.
Comment 7 Mike Hommey [:glandium] 2012-01-25 02:30:37 PST
Created attachment 591396 [details] [diff] [review]
part 1 - Make Mappable::munmap, Mappable1stPagePtr::munmap and MappedPtr::munmap private
Comment 8 Mike Hommey [:glandium] 2012-01-25 02:33:15 PST
Created attachment 591397 [details] [diff] [review]
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space
Comment 9 Mike Hommey [:glandium] 2012-01-25 06:34:42 PST
Created attachment 591443 [details] [diff] [review]
part 1 - Make Mappable::munmap, Mappable1stPagePtr::munmap and MappedPtr::munmap private
Comment 10 Mike Hommey [:glandium] 2012-01-25 06:46:55 PST
Created attachment 591450 [details] [diff] [review]
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space
Comment 11 (dormant account) 2012-01-25 14:18:43 PST
Comment on attachment 591450 [details] [diff] [review]
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space

rubberstamp
Comment 12 Mike Hommey [:glandium] 2012-01-26 00:09:08 PST
Comment on attachment 591450 [details] [diff] [review]
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space

Sadly, this patch breaks the crash reporter on Android. Apparently, the dump process gets a SIGILL, which brings it back in the signal handler, and dead locks on a mutex in breakpad.
Comment 13 Mike Hommey [:glandium] 2012-01-26 02:21:51 PST
(In reply to Mike Hommey [:glandium] from comment #12)
> Sadly, this patch breaks the crash reporter on Android. Apparently, the dump
> process gets a SIGILL, which brings it back in the signal handler, and dead
> locks on a mutex in breakpad.

It was in fact a bug in breakpad.
Comment 14 Mike Hommey [:glandium] 2012-01-26 06:46:33 PST
Created attachment 591776 [details] [diff] [review]
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space

This essentially changes one thing:

-      elf->mappable->prefetch(info->si_addr);
-      return;
+      if (elf->mappable->prefetch(info->si_addr))
+        return;

This allows to rethrow the segmentation fault if the linker couldn't
handle it, instead of deadlocking.
Comment 15 Mike Hommey [:glandium] 2012-01-26 11:23:38 PST
Created attachment 591871 [details] [diff] [review]
WIP part 3 & 4

This combines what will be part 3 and 4 once everything is cleaned up (especially comments, some don't even match what's happening anymore). This started off "Rebase against bug 683127 patches of 6 Jan (apply second)", replaced some types, used some things from Utils.h, made it more C++ish, and removed the background decompression, which in the end, was not a win. Decompression is now within the handler as discussed on IRC.
At this point, I think the remaining changes are mainly cosmetic, so I'm seeking for pre-review, here.
I'll push to try to get a build for people to test on different devices.
Comment 16 Mike Hommey [:glandium] 2012-01-26 11:29:49 PST
Ah, one thing I want to change is the mprotect in MappableSZ::handle_fault, to make it apply on everything that is decompressed instead of just the page that faulted. And I want to add an equivalent to MappableExtractFile (which would extract the stream in a file, completely, and then map the file, for the debug intent)
Comment 18 Julian Seward [:jseward] 2012-01-27 05:52:31 PST
Comment on attachment 591871 [details] [diff] [review]
WIP part 3 & 4

Seems fine.  Some minor comments:

+      } else if (s.GetType() == Zip::Stream::STORE) {
+        mappable = MappableSZ::Create(name, zip, &s);

This logic assumes that DEFLATE vs STORE is a reliable indication
of non-seekable vs seekable zip.  I guess that's OK since it's
very unlikely that a compress attempt for a seekable zip would
find it to be so uncompressable that it would up being STOREd.


+SZStream::Init (void* ptr)

General comment: I think there may be unaligned accesses when
fetching bits of the seekable zstream.  Is that OK?  I think they
are ok on all important archs (x86, x86_64, armv6 and later)


+   strm.avail_in  = 999999; // FIXME

This assumes that the (un)compressed block size <= 999999;
I suppose that's OK.


+// This is the core of the thing.  It contains the segfault handler
+// and the decompression function.  These run in separate threads.

(eg) some of the comments are a bit out of date now, considering there
is no separate decompress thread.
Comment 19 Mike Hommey [:glandium] 2012-01-27 06:13:56 PST
(In reply to Julian Seward from comment #18)
> Comment on attachment 591871 [details] [diff] [review]
> WIP part 3 & 4
> 
> Seems fine.  Some minor comments:
> 
> +      } else if (s.GetType() == Zip::Stream::STORE) {
> +        mappable = MappableSZ::Create(name, zip, &s);
> 
> This logic assumes that DEFLATE vs STORE is a reliable indication
> of non-seekable vs seekable zip.  I guess that's OK since it's
> very unlikely that a compress attempt for a seekable zip would
> find it to be so uncompressable that it would up being STOREd.

Seekable zips are *not* meant to be DEFLATEd, and plain libraries are *not* meant to be STOREd. Both cases won't work, they're supported by no code. They will gracefully fail as well, since the ::Create methods will properly fail.

> +SZStream::Init (void* ptr)
> 
> General comment: I think there may be unaligned accesses when
> fetching bits of the seekable zstream.  Is that OK?  I think they
> are ok on all important archs (x86, x86_64, armv6 and later)

The le_uint32 type used in SZSHeader is automagically read properly.

> 
> 
> +   strm.avail_in  = 999999; // FIXME
> 
> This assumes that the (un)compressed block size <= 999999;
> I suppose that's OK.

I guess we could use avail_out * 2 to make it more ok.

> +// This is the core of the thing.  It contains the segfault handler
> +// and the decompression function.  These run in separate threads.
> 
> (eg) some of the comments are a bit out of date now, considering there
> is no separate decompress thread.

Yeah, that's what I wrote in comment 15, a lot of the comments need to be changed :)
Comment 20 Mike Hommey [:glandium] 2012-02-01 05:27:48 PST
Comment on attachment 591776 [details] [diff] [review]
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space

carrying over r+, which i had forgotten to do.
Comment 21 Mike Hommey [:glandium] 2012-02-09 08:06:43 PST
Created attachment 595750 [details] [diff] [review]
part 3 - Tool to generate seekable compressed streams
Comment 22 Mike Hommey [:glandium] 2012-02-09 08:08:17 PST
Created attachment 595751 [details] [diff] [review]
part 3 - Tool to generate seekable compressed streams
Comment 23 (dormant account) 2012-02-09 16:30:16 PST
Comment on attachment 595751 [details] [diff] [review]
part 3 - Tool to generate seekable compressed streams

I am going to assume the le_to_cpu stuff just works.


+  if (argc != 3 || !argv[1] || !argv[2] || (strcmp(argv[1], argv[2]) == 0)) {
+    char *lastSlash = rindex(argv[0], '/');
+    log("usage: %s file_to_compress out_file",
+                    lastSlash ? lastSlash + 1 : argv[0]);
+    return 1;
+  }

Really? Is it so bad to display a relative/absolute filename in an error?

+  struct stat st;
+  int ret = fstat(unzF, &st);
+  if (ret == -1) {
+    log("Couldn't seek %s: %s", argv[1], strerror(errno));
+    return 1;
+  }

wrong error message

+  /* Give enough room for the header and the offset table, and map them */
+  ftruncate(outF, zOff)

In theory fallocate is more efficient for this.


The variable naming in this patch is terrible:szsh(can we use zHeader or something?), SZSHeader(don't use abbrevs in type names please), s/outF/outFD/, strm(we aren't Egyptians, vowels are encouraged).

+  static const uint32_t magic; // = 0x7a5a6553;
+                                /* 'S' 'e' 'Z' - Seekable Zstream
+                                 * 'z'         - chunks compressed by zlib. */
+
Are we inventing a new zip stream format? ie is this a custom new magic?


+#pragma pack(1) <-- is this supposed to compile on windows, if not I prefer to use the GNU packed attribute on the class.
Comment 24 Mike Hommey [:glandium] 2012-02-09 23:18:40 PST
(In reply to Taras Glek (:taras) from comment #23)
> The variable naming in this patch is terrible:szsh(can we use zHeader or
> something?), SZSHeader(don't use abbrevs in type names please),
> s/outF/outFD/, strm(we aren't Egyptians, vowels are encouraged).

Looks like I didn't change enough variable names from Julian's original implementation. :)

> +  static const uint32_t magic; // = 0x7a5a6553;
> +                                /* 'S' 'e' 'Z' - Seekable Zstream
> +                                 * 'z'         - chunks compressed by zlib.
> */
> +
> Are we inventing a new zip stream format? ie is this a custom new magic?

It's just a file format that happens to start with a magic number, and there is code to handle that in Zip.h, so I though it was better to reuse it.
 
> +#pragma pack(1) <-- is this supposed to compile on windows, if not I prefer
> to use the GNU packed attribute on the class.

It's not supposed to compile on windows now, but may some day. Zip.h also uses pragma pack, it makes things consistent at the faulty.lib level.
Comment 25 Julian Seward [:jseward] 2012-02-10 00:17:12 PST
Comment on attachment 595751 [details] [diff] [review]
part 3 - Tool to generate seekable compressed streams

Looks ok from a functional point of view.  Minor paranoia: in the
chunk compression loop, maybe assert that we didn't overrun the
output buffer for any reason?  Since it is sized as 2 * CHUNK we
should never get anywhere near zero for strm.avail_out.

+    ret = deflate(&strm, Z_FINISH);
+    MOZ_ASSERT(ret == Z_STREAM_END);
     MOZ_ASSERT(strm.avail_out > 0);
Comment 26 (dormant account) 2012-02-10 15:33:13 PST
(In reply to Mike Hommey [:glandium] from comment #24)
> (In reply to Taras Glek (:taras) from comment #23)
> > The variable naming in this patch is terrible:szsh(can we use zHeader or
> > something?), SZSHeader(don't use abbrevs in type names please),
> > s/outF/outFD/, strm(we aren't Egyptians, vowels are encouraged).
> 
> Looks like I didn't change enough variable names from Julian's original
> implementation. :)

I suspected that, please change them. Whatever usual naming convention you use is ok.
> 
> > +  static const uint32_t magic; // = 0x7a5a6553;
> > +                                /* 'S' 'e' 'Z' - Seekable Zstream
> > +                                 * 'z'         - chunks compressed by zlib.
> > */
> > +
> > Are we inventing a new zip stream format? ie is this a custom new magic?
> 
> It's just a file format that happens to start with a magic number, and there
> is code to handle that in Zip.h, so I though it was better to reuse it.

I was curious if this was a brand new magic number. sounds like a yes.
>  
> > +#pragma pack(1) <-- is this supposed to compile on windows, if not I prefer
> > to use the GNU packed attribute on the class.
> 
> It's not supposed to compile on windows now, but may some day. Zip.h also
> uses pragma pack, it makes things consistent at the faulty.lib level.

ok.
Comment 27 Mike Hommey [:glandium] 2012-02-15 01:32:28 PST
Created attachment 597338 [details] [diff] [review]
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space

This only changes prefetch name and signature.
Comment 28 Mike Hommey [:glandium] 2012-02-15 01:34:34 PST
Comment on attachment 597338 [details] [diff] [review]
part 2 - Use a SIGSEGV signal handler to handle segmentation faults happening in loaded libraries address space

Carrying over r+
Comment 29 Mike Hommey [:glandium] 2012-02-15 01:35:28 PST
Created attachment 597339 [details] [diff] [review]
part 3 - Tool to generate seekable compressed streams
Comment 30 Mike Hommey [:glandium] 2012-02-15 01:41:34 PST
Created attachment 597340 [details] [diff] [review]
part 4 - Make the linker load libraries with on-demand decompression when they are seekable compressed streams

This is quite different from the previous iterations. While jseward's implementation was meant to be generic and may have been used for mostly anything besides the linker, this version takes some shortcuts due to what we know about libraries. It basically makes the same assumptions as the one made in MappableDeflate.
There is also an implementation of full extraction for the debug intent.
Comment 31 Mike Hommey [:glandium] 2012-02-15 02:30:30 PST
Created attachment 597349 [details] [diff] [review]
part 5 - Optionally make libxul.so a seekable compressed stream on Android

This doesn't enable building with seekable compressed streams by default. One needs to export SZIP_LIBRARIES=1 in .mozconfig in order to actually enable it.
Comment 32 Julian Seward [:jseward] 2012-02-15 04:21:19 PST
Comment on attachment 597340 [details] [diff] [review]
part 4 - Make the linker load libraries with on-demand decompression when they are seekable compressed streams

What is it that MappableSeekableZStream::mutex protects?  
It appears to protect ::chunkAvail, but shouldn't it also protect
::lazyMaps ?  I don't see what stops
MappableSeekableZStream::{mmap,munmap} messing with ::lazyMaps whilst
MappableSeekableZStream::ensure reads it in a different thread.



+   * When we fault in the remaining pages of that chunk, we want to decompress
+   * the complete chunk again */

we want -> we don't want ?



+#if defined(ANDROID) && defined(__arm__)
+    if (map->prot & PROT_EXEC) {
+      /* We just extracted data that may be executed in the future.
+       * We thus need to ensure Instruction and Data cache coherency. */

isn't this really just "#if defined(__arm__)" ?  Although I guess the
cacheflush call calling conventions etc might be android specific.
Comment 33 Mike Hommey [:glandium] 2012-02-15 04:31:23 PST
(In reply to Julian Seward from comment #32)
> Comment on attachment 597340 [details] [diff] [review]
> part 4 - Make the linker load libraries with on-demand decompression when
> they are seekable compressed streams
> 
> What is it that MappableSeekableZStream::mutex protects?  
> It appears to protect ::chunkAvail, but shouldn't it also protect
> ::lazyMaps ?  I don't see what stops
> MappableSeekableZStream::{mmap,munmap} messing with ::lazyMaps whilst
> MappableSeekableZStream::ensure reads it in a different thread.

What stops it from happening is the way the linker works. This is one of the shortcuts we can safely take in a non-generic implementation.

> +   * When we fault in the remaining pages of that chunk, we want to
> decompress
> +   * the complete chunk again */
> 
> we want -> we don't want ?

we want. If we didn't, we'd end up with nothing between 4k and 16k (which in android libxul.so case would mean a corrupted DT_HASH table).

> +#if defined(ANDROID) && defined(__arm__)
> +    if (map->prot & PROT_EXEC) {
> +      /* We just extracted data that may be executed in the future.
> +       * We thus need to ensure Instruction and Data cache coherency. */
> 
> isn't this really just "#if defined(__arm__)" ?  Although I guess the
> cacheflush call calling conventions etc might be android specific.

The cacheflush function is android specific, and since the code for the corresponding cache flush is not there...
Comment 34 Julian Seward [:jseward] 2012-02-15 05:42:15 PST
Comment on attachment 597340 [details] [diff] [review]
part 4 - Make the linker load libraries with on-demand decompression when they are seekable compressed streams

> > What is it that MappableSeekableZStream::mutex protects?  
> > It appears to protect ::chunkAvail, but shouldn't it also protect
> > ::lazyMaps ?  I don't see what stops
> > MappableSeekableZStream::{mmap,munmap} messing with ::lazyMaps whilst
> > MappableSeekableZStream::ensure reads it in a different thread.
> 
> What stops it from happening is the way the linker works. This is one of the shortcuts we can safely take > in a non-generic implementation.

Maybe add a comment explaining how the linker's design makes this
safe, for the benefit of future visitors to this code.
Comment 35 Mike Hommey [:glandium] 2012-02-15 06:54:26 PST
Created attachment 597389 [details] [diff] [review]
part 6 - Add functions to display stats about seekable compressed streams
Comment 36 Mike Hommey [:glandium] 2012-02-15 06:55:23 PST
Created attachment 597391 [details] [diff] [review]
part 6 - Add functions to display stats about seekable compressed streams
Comment 37 Mike Hommey [:glandium] 2012-02-15 06:56:19 PST
Created attachment 597393 [details] [diff] [review]
part 7 - Hook linker stats in Startup Timeline
Comment 38 Mike Hommey [:glandium] 2012-02-15 06:57:14 PST
Comment on attachment 597393 [details] [diff] [review]
part 7 - Hook linker stats in Startup Timeline

For the build bits.
Comment 39 Brad Lassey [:blassey] (use needinfo?) 2012-02-15 09:44:34 PST
Comment on attachment 597349 [details] [diff] [review]
part 5 - Optionally make libxul.so a seekable compressed stream on Android

khuey would be a better reviewer
Comment 40 (dormant account) 2012-02-15 10:56:20 PST
Comment on attachment 597340 [details] [diff] [review]
part 4 - Make the linker load libraries with on-demand decompression when they are seekable compressed streams

This looks nice, not much room for improvements, thus r+. 

+    debug("%s: Calling function @%p", GetPath(), ptr);
     f.func();

leftover debug code?

MappableExtractFile::Create <-- worried about logic duplication here(OO would be safer). could lead to badness in the future. According to IRC this code will go away in a future iteration.


+  size_t chunkEnd = chunkStart + length;
+  std::vector<LazyMap>::iterator it;
+  for (it = map; it < lazyMaps.end(); ++it) {
+    if (chunkEnd <= it->offset + it->length)
+      break;
+  }
+  if ((it == lazyMaps.end()) || (chunkEnd > it->offset + it->length)) {
+    /* The mapping "it" points at now is past the interesting one */
+    --it;
+    length = it->offset + it->length - chunkStart;
+  }

it->offset + it->length used multiple times... Use a casted ->end()..or add a new utility accessor?

i think LazyMap could also use a contains() method 


I would prefer raii mutex usage.

Please comment finalize/ensure in Mappable.h.
Comment 41 (dormant account) 2012-02-15 11:03:14 PST
Comment on attachment 597391 [details] [diff] [review]
part 6 - Add functions to display stats about seekable compressed streams

+  for (size_t i = 0, j = 1; i < nEntries; i++, j++) {
+    map[j] = chunkAvail[i] ? '*' : '_';
+    if ((j == len) || (i == nEntries - 1)) {
+      map[j + 1] = ']';
+      map[j + 2] = '\0';
+      debug("%s", static_cast<char *>(map));
+      j = 0;
+    }
+  }

I think this is missing a "break;" to avoid weird overwriting due to j being reset to 0.
Comment 42 (dormant account) 2012-02-15 11:26:30 PST
Comment on attachment 597393 [details] [diff] [review]
part 7 - Hook linker stats in Startup Timeline

I expected this patch to be the inverse:ie expose some of the linker timestamps via telemetry.
Comment 43 Mike Hommey [:glandium] 2012-02-15 11:28:38 PST
(In reply to Taras Glek (:taras) from comment #41)
> I think this is missing a "break;" to avoid weird overwriting due to j being
> reset to 0.

Overwriting is the wanted effect.
Comment 44 Mike Hommey [:glandium] 2012-02-15 11:31:15 PST
(In reply to Taras Glek (:taras) from comment #40)
> Comment on attachment 597340 [details] [diff] [review]
> part 4 - Make the linker load libraries with on-demand decompression when
> they are seekable compressed streams
> 
> This looks nice, not much room for improvements, thus r+. 
> 
> +    debug("%s: Calling function @%p", GetPath(), ptr);
>      f.func();
> 
> leftover debug code?

mmm yes and no. This probably doesn't have its place here, and as such it is a leftover, on the other hand, it's a useful debug output that should have been here in the first place.

> Please comment finalize/ensure in Mappable.h.

They are described in Mappable, where they are inherited from. Isn't that enough?
Comment 45 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-02-15 11:31:31 PST
Comment on attachment 597349 [details] [diff] [review]
part 5 - Optionally make libxul.so a seekable compressed stream on Android

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

::: toolkit/mozapps/installer/packager.mk
@@ +308,5 @@
>    removed-files \
>    recommended-addons.json \
>    $(NULL)
>  
> +ifdef SZIP_LIBRARIES

Is this supposed to be ifndef?
Comment 46 Mike Hommey [:glandium] 2012-02-15 13:29:56 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #45)
> Comment on attachment 597349 [details] [diff] [review]
> part 5 - Optionally make libxul.so a seekable compressed stream on Android
> 
> Review of attachment 597349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/installer/packager.mk
> @@ +308,5 @@
> >    removed-files \
> >    recommended-addons.json \
> >    $(NULL)
> >  
> > +ifdef SZIP_LIBRARIES
> 
> Is this supposed to be ifndef?

it's supposed to be ifdef. Maybe using the same variable is confusing though. Would you rather I use a different variable?
Comment 47 (dormant account) 2012-02-15 13:58:13 PST
(In reply to Mike Hommey [:glandium] from comment #44)

> > Please comment finalize/ensure in Mappable.h.
> 
> They are described in Mappable, where they are inherited from. Isn't that
> enough?

it's enough.
Comment 48 Mike Hommey [:glandium] 2012-02-16 04:09:37 PST
Created attachment 597769 [details] [diff] [review]
part 4 - Make the linker load libraries with on-demand decompression when they are seekable compressed streams
Comment 49 Mike Hommey [:glandium] 2012-02-16 04:10:38 PST
Created attachment 597770 [details] [diff] [review]
part 6 - Add functions to display stats about seekable compressed streams

Refreshed after new part 4.
Comment 50 Mike Hommey [:glandium] 2012-02-16 04:12:09 PST
Comment on attachment 597770 [details] [diff] [review]
part 6 - Add functions to display stats about seekable compressed streams

Carrying over r+
Comment 51 Mike Hommey [:glandium] 2012-02-16 06:06:42 PST
(In reply to Taras Glek (:taras) from comment #42)
> Comment on attachment 597393 [details] [diff] [review]
> part 7 - Hook linker stats in Startup Timeline
> 
> I expected this patch to be the inverse:ie expose some of the linker
> timestamps via telemetry.

I'll change the commit message when landing.
Comment 52 Mike Hommey [:glandium] 2012-02-16 07:27:19 PST
Created attachment 597809 [details] [diff] [review]
part 5 - Optionally make libxul.so a seekable compressed stream on Android
Comment 53 Mike Hommey [:glandium] 2012-02-16 07:34:24 PST
Created attachment 597810 [details] [diff] [review]
part 5 - Optionally make libxul.so a seekable compressed stream on Android

Adding MOZ_ to the config variable, per ted suggestion on irc.
Comment 54 Mike Hommey [:glandium] 2012-02-16 07:35:09 PST
Comment on attachment 597810 [details] [diff] [review]
part 5 - Optionally make libxul.so a seekable compressed stream on Android

Carrying over r+.
Comment 55 Mike Hommey [:glandium] 2012-02-16 08:53:11 PST
Created attachment 597841 [details] [diff] [review]
part 7 - Display linker stats on each Startup Timeline event

Refreshed after part 5
Comment 56 Mike Hommey [:glandium] 2012-02-16 08:56:54 PST
Comment on attachment 597841 [details] [diff] [review]
part 7 - Display linker stats on each Startup Timeline event

Carrying r+ over.
Comment 57 (dormant account) 2012-02-21 15:43:11 PST
Comment on attachment 597769 [details] [diff] [review]
part 4 - Make the linker load libraries with on-demand decompression when they are seekable compressed streams


+  if (inflate(&zStream, (length == chunkLen) ? Z_FINISH : Z_SYNC_FLUSH)
+      != (length == chunkLen) ? Z_STREAM_END : Z_OK) {

bit ugly.

Sorry, I forgot to click submit on friday :(

Note You need to log in before you can comment on or make changes to this bug.