Add linker support for LZMA-compressed shared libraries

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

(Blocks 1 bug)

51 Branch
Firefox 52
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 52+, firefox51 affected, firefox52 fixed)

Details

Attachments

(3 attachments, 9 obsolete attachments)

1.87 KB, patch
snorp
: review+
glandium
: review+
glandium
: feedback+
Details | Diff | Splinter Review
7.50 KB, patch
glandium
: review+
Details | Diff | Splinter Review
846 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
Currently, shared libraries are compressed with szip which allows for seekable stream on-demand decompression.

In bug 1291424 we look into disabling on-demand decompression which would allow us to use more aggressive compression on the libraries.
To enable XZ decoding, we can conveniently ship with the lightweight XZ Embedded [1] library.

I've added the required unmodified files directly to mozglue/linker with the original copyright notice intact. We could move it to its own directory and/or ship with its README etc. Any preferred way here?

[1] http://git.tukaani.org/xz-embedded.git
Attachment #8783987 - Flags: feedback?(snorp)
Attachment #8783987 - Flags: feedback?(mh+mozilla)
This adds an interface class to help with the C interface of XZ Embedded.
A critical part here is the choice of the LZMA dictionary size and allocation mechanics.

The setting used is suitable for -5 and -6 compression level presets, -6 being the default.
Attachment #8783990 - Flags: feedback?(snorp)
Attachment #8783990 - Flags: feedback?(mh+mozilla)
Finally, with this we use XZStream to detect and decode XZ streams.

XZ Utils doesn't write the uncompressed size into the XZ block headers, so we can't depend on it during decoding.
Instead, we guess the uncompressed size (currently set to 4x compressed size) and then trim the file accordingly once we have finished decoding.

I've found that we actually fail to mmap the buffer for the second time, should we underestimate the uncompressed size in which case we ftruncate the file to the new extended estimate and try to re-mmap the based on the new size and offset. Any ideas why that would happen?
Attachment #8783993 - Flags: feedback?(snorp)
Attachment #8783993 - Flags: feedback?(mh+mozilla)
For testing, this will force-compress libraries with XZ.
Do we want to add this as an additional option or replace szip?

What's the proper way of adding XZ Utils as a dependency?
Attachment #8784012 - Flags: feedback?(mh+mozilla)
Test results on a Pixel C with debug symbols:

szip
libxul.so: 29MB
initial extraction time: 1s

xz
libxul.so: 22MB
initial extraction time: 4s
4 seconds might be OK for release users, once every 3-6 weeks, with an "Upgrading your Firefox experience…" spinner. Perhaps not OK for Nightly users; first launch every day would get tiring.

Can we extract in the background on upgrade by listening for MY_PACKAGE_REPLACED[1], and show a spinner on cold start extract?

7MB is a pretty huge difference…

What's the delta without debug symbols?


[1] <https://developer.android.com/reference/android/content/Intent.html#ACTION_MY_PACKAGE_REPLACED>
Running xz on already xz-compressed files seems to add additional xz headers which breaks decoding. I've updated packaging to check for it before compressing.
Attachment #8784012 - Attachment is obsolete: true
Attachment #8784012 - Flags: feedback?(mh+mozilla)
Attachment #8783987 - Flags: feedback?(snorp) → feedback+
Test results on a Pixel C *without* debug symbols:

szip
libxul.so: 20MB
initial extraction time: 0.7s

xz
libxul.so: 15MB
initial extraction time: 3s
Comment on attachment 8783990 [details] [diff] [review]
0002-Bug-1294736-2.1-Implement-XZStream-interface-for-dec.patch

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

Looks pretty good, a few nits

::: mozglue/linker/XZStream.cpp
@@ +67,5 @@
> +#endif
> +
> +    switch (ret) {
> +      case XZ_STREAM_END:
> +        // Stream ended, the next loop iteration should terminate.

I guess it should terminate because the 'while' condition fails? Maybe something more explicit here would be good.

Also, does this mean that we only decompressed a portion of what was requested? Is that expected to occur normally? Seems like logging an error would make sense here too.

@@ +87,5 @@
> +        ERROR("XZ decoding: unsupported header options");
> +        return 0;
> +
> +      case XZ_DATA_ERROR:
> +        // Fallthrough

MOZ_FALLTHROUGH

::: mozglue/linker/XZStream.h
@@ +14,5 @@
> +{
> +public:
> +  static bool IsXZ(const void* aBuf);
> +
> +  XZStream(const void* aInBuf, size_t aInLen);

void* seems weird here, maybe just use uint8_t* since that's what you cast to anyway?

@@ +18,5 @@
> +  XZStream(const void* aInBuf, size_t aInLen);
> +  ~XZStream();
> +
> +  bool Init();
> +  size_t Decode(void* aOutBuf, size_t aOutLen);

Same (void* -> uint8_t*)
Attachment #8783990 - Flags: feedback?(snorp) → feedback+
Comment on attachment 8783990 [details] [diff] [review]
0002-Bug-1294736-2.1-Implement-XZStream-interface-for-dec.patch

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

::: mozglue/linker/XZStream.cpp
@@ +6,5 @@
> +
> +/* static */ bool
> +XZStream::IsXZ(const void* aBuf)
> +{
> +  static const uint8_t xzMagic[] = {0xfd, '7', 'z', 'X', 'Z', 0x0};

xz_stream.h has a HEADER_MAGIC that you could reuse maybe
Comment on attachment 8783993 [details] [diff] [review]
0003-Bug-1294736-3.1-Detect-and-decode-XZ-streams-when-ex.patch

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

::: mozglue/linker/Mappable.cpp
@@ +196,5 @@
> +    size_t outLen = 0;
> +    size_t written = 0;
> +    size_t inLeft = 0;
> +
> +    while ((inLeft = xzStream.RemainingInput())) {

I am almost certain there is a way to get the uncompressed size from the archive. That would simplify this a bit, IMHO.

@@ +197,5 @@
> +    size_t written = 0;
> +    size_t inLeft = 0;
> +
> +    while ((inLeft = xzStream.RemainingInput())) {
> +      if (written + inLeft > outLen) {

I'm not sure what's going on here. You initialize outLen to 0, but then only increment it inside of this block.
Attachment #8783993 - Flags: feedback?(snorp) → feedback-
Switching the compression level presets doesn't change much in terms of size or decoding duration.
Disabling the content verification (CRC32) reduces decoding by ~200ms (non-debug).

I'll look into other options that may affect performance.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #11)
> I am almost certain there is a way to get the uncompressed size from the
> archive. That would simplify this a bit, IMHO.

There is, we should be able to extract the uncompressed size from the index. My first approach was to extract it from the block headers, but XZ Utils doesn't provide that optional field. This is something I'll look into, but I wanted to have some feedback on the rest.

> I'm not sure what's going on here. You initialize outLen to 0, but then only
> increment it inside of this block.

This way I avoid writing out the redundant initial ftruncate.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #9)
> I guess it should terminate because the 'while' condition fails? Maybe
> something more explicit here would be good.

Correct, I will adjust the comment.

> Also, does this mean that we only decompressed a portion of what was
> requested? Is that expected to occur normally? Seems like logging an error
> would make sense here too.

XZ_STREAM_END is returned when we have successfully finished decoding, it's not an error condition.

> void* seems weird here, maybe just use uint8_t* since that's what you cast
> to anyway?

It's just a matter of casting at the call site vs. casting in the implementation. The choice to uint8_t is really arbitrary, since any byte-sized type would work. At the interface level, we really don't care about the underlying buffer type, so void* seemed like the cleaner choice.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10) 
> xz_stream.h has a HEADER_MAGIC that you could reuse maybe

It's probably safer to not depend on anything outside of the API defined in xz.h to allow for easy updating of the xz lib.
Knowing the expected uncompressed size, we can simplify the buffer/file handing.
Attachment #8783993 - Attachment is obsolete: true
Attachment #8783993 - Flags: feedback?(mh+mozilla)
Attachment #8784532 - Flags: review?(snorp)
Attachment #8784532 - Flags: review?(mh+mozilla)
Addressed comments.
Simplified the decode loop.
Added uncompressed size parsing to allow for pre-allocation of output buffers.

None of the functionality can be easily extracted from XZ Embedded, the implementation is based on the latest stable xz 1.0.4 (2009-08-27) spec [1].

[1] http://tukaani.org/xz/xz-file-format-1.0.4.txt
Attachment #8784531 - Attachment is obsolete: true
Attachment #8784531 - Flags: review?(snorp)
Attachment #8784531 - Flags: review?(mh+mozilla)
Attachment #8784563 - Flags: review?(snorp)
Attachment #8784563 - Flags: review?(mh+mozilla)
Depends on: 1298090
Attachment #8784532 - Flags: review?(snorp) → review+
Attachment #8784563 - Flags: review?(snorp) → review+
Comment on attachment 8783987 [details] [diff] [review]
0001-Bug-1294736-1.1-Add-XZ-Embedded-to-enable-XZ-decodin.patch

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

This should go in its own directory, probably along zlib, bzip2 and brotli under modules/. This may also require updating toolkit/content/license.html, but this is public domain, I don't know what our policy is for those wrt license.html. Gerv?
Attachment #8783987 - Flags: feedback?(mh+mozilla) → feedback?(gerv)
Comment on attachment 8784532 [details] [diff] [review]
0003-Bug-1294736-3.2-Detect-and-decode-XZ-streams-when-ex.patch

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

::: mozglue/linker/Mappable.cpp
@@ +194,5 @@
>            static_cast<unsigned int>(stream->GetUncompressedSize()));
>        return nullptr;
>      }
> +  } else if (XZStream::IsXZ(stream->GetBuffer(), stream->GetSize())) {
> +    XZStream xzStream(stream->GetBuffer(), stream->GetSize());

The whole thing is, at a high level, all the same for deflate, szip, and now xz. It would be desirable to have a common base class with virtual methods, and have the decompression logic all shared between the three.
Attachment #8784532 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8784563 [details] [diff] [review]
0002-Bug-1294736-2.2-Implement-XZStream-interface-for-dec.patch

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

::: mozglue/linker/XZStream.cpp
@@ +3,5 @@
> +#include <algorithm>
> +#include "mozilla/Assertions.h"
> +#include "Logging.h"
> +
> +// LZMA dictionary size, should have a minium size for the given compression

typo: minimum.

@@ +26,5 @@
> +  while (aBuf[i++] & 0x80) {
> +    if (i >= aBufSize || aBuf[i] == 0x0) {
> +      return 0;
> +    }
> +    *aValue |= static_cast<uint64_t>(aBuf[i] & 0x7F) << (i * 7);

This could be written as:

  *aValue = 0;
  size_t i;
  for (i = 0; i <= aBufSize && aBuf[i]; i++) {
    *aValue |= static_cast<uint64_t)(aBuf[i] & 0x7f) << (i * 7);
    if (!aBuf[i] & 0x80) {
        break;
    }
  }
  return i;

@@ +42,5 @@
> +}
> +
> +XZStream::XZStream(const void* aInBuf, size_t aInSize)
> +  : mInBuf(static_cast<const uint8_t*>(aInBuf))
> +  , mUncompSize(0)

You're not initializing mDec. If for some reason Init is not called before the destructor, you're going to call xz_dec_end with a garbage pointer.

@@ +121,5 @@
> +        ERROR("XZ decoding: corrupt input stream");
> +        return 0;
> +
> +      default:
> +        MOZ_ASSERT_UNREACHABLE("XZ decoding: unkown error condition");

typo: unknown

@@ +195,5 @@
> +  index += ParseVarLenInt(index, end - index, &numRecords);
> +  if (!numRecords) {
> +    return 0;
> +  }
> +  MOZ_ASSERT(numRecords == 1);

The assertion is that the number of records match the number of blocks. While we'd expect there is only one block, it's still better to check we processed the right number of blocks rather than hardcode that there always is only one block.

::: mozglue/linker/XZStream.h
@@ +20,5 @@
> +  // Creates a XZ stream object for the given input buffer.
> +  XZStream(const void* aInBuf, size_t aInSize);
> +  ~XZStream();
> +
> +  // Initializes the decoder and returns whether deocding may commence.

typo: decoding
Attachment #8784563 - Flags: review?(mh+mozilla) → feedback+
(In reply to Mike Hommey [:glandium] from comment #17)
> This should go in its own directory, probably along zlib, bzip2 and brotli
> under modules/. This may also require updating toolkit/content/license.html,
> but this is public domain, I don't know what our policy is for those wrt
> license.html. Gerv?

No need to update license.html.

Gerv
Moved the library to mobules/xz-embedded.

Do we need any files there for documentation?
Attachment #8783987 - Attachment is obsolete: true
Attachment #8795462 - Flags: review?(mh+mozilla)
Addressed comments unless noted below.

(In reply to Mike Hommey [:glandium] from comment #19)
> @@ +26,5 @@
> > +  while (aBuf[i++] & 0x80) {
> > +    if (i >= aBufSize || aBuf[i] == 0x0) {
> > +      return 0;
> > +    }
> > +    *aValue |= static_cast<uint64_t>(aBuf[i] & 0x7F) << (i * 7);
> 
> This could be written as:
> 
>   *aValue = 0;
>   size_t i;
>   for (i = 0; i <= aBufSize && aBuf[i]; i++) {
>     *aValue |= static_cast<uint64_t)(aBuf[i] & 0x7f) << (i * 7);
>     if (!aBuf[i] & 0x80) {
>         break;
>     }
>   }
>   return i;

We have to return 0 if 0x0 is encountered or we're out of buffer bounds before reading a terminating byte (x & 0x80 == 0 && x != 0x0). Counter example: aBuf = [0x80, 0x00].

> @@ +195,5 @@
> > +  index += ParseVarLenInt(index, end - index, &numRecords);
> > +  if (!numRecords) {
> > +    return 0;
> > +  }
> > +  MOZ_ASSERT(numRecords == 1);
> 
> The assertion is that the number of records match the number of blocks.
> While we'd expect there is only one block, it's still better to check we
> processed the right number of blocks rather than hardcode that there always
> is only one block.

I've removed the assertion since we're checking for the size before decoding blocks. The decoder will verify the data integrity, we shouldn't do it at this stage.
Attachment #8784563 - Attachment is obsolete: true
Attachment #8795468 - Flags: review?(mh+mozilla)
Comment on attachment 8784532 [details] [diff] [review]
0003-Bug-1294736-3.2-Detect-and-decode-XZ-streams-when-ex.patch

(In reply to Mike Hommey [:glandium] from comment #18)
> The whole thing is, at a high level, all the same for deflate, szip, and now
> xz. It would be desirable to have a common base class with virtual methods,
> and have the decompression logic all shared between the three.

Yes, I agree. Are you OK with me moving the refactoring out of this bug and just merge it with the ElfLoader refactoring to have a separated discussion?

I'm trying to keep the linker fixes and feature impl. separted from the refactoring work for easier uplifting.
Attachment #8784532 - Flags: review?(mh+mozilla)
We can either suppress compiler warning in patch 1 or we need this to not break our builds with xz-embedded.
Attachment #8795475 - Flags: review?(mh+mozilla)
Summary: Investigate more aggressive compression of shared libraries → Add linker support for LZMA-compressed shared libraries
Comment on attachment 8795462 [details] [diff] [review]
0001-Bug-1294736-1.2-Add-XZ-Embedded-to-enable-XZ-decodin.patch

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

Please add a README.mozilla that says where the code was taken from, and what version it is (the answer to which seems to be http://git.tukaani.org/xz-embedded.git and e75f4eb79165213a02d567940d344f5c2ff1be03). Even better if you add a script that copies the right bits into the right place since this obviously doesn't match xz-embedded upstream directory structure.

You also have an extra '\n' at the end of xz.h, compared to upstream.

::: config/external/moz.build
@@ +60,4 @@
>      'media/libsoundtouch',
>  ]
>  
> +external_dirs += ['modules/xz-embedded']

This should be gated on MOZ_LINKER.
Attachment #8795462 - Flags: review?(mh+mozilla)
Attachment #8795468 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8795468 [details] [diff] [review]
0002-Bug-1294736-2.3-Implement-XZStream-interface-for-dec.patch

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

::: mozglue/linker/XZStream.h
@@ +10,5 @@
> +#define XZ_DEC_DYNALLOC
> +#include "xz.h"
> +
> +// Used to decode XZ stream buffers.
> +class XZStream

Note, this class has needless differences with SeekableZStream, which make the code in part 3 have needless differences with the corresponding code for SeekableZStream.
Comment on attachment 8784532 [details] [diff] [review]
0003-Bug-1294736-3.2-Detect-and-decode-XZ-streams-when-ex.patch

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

See comment on part 2.
Attachment #8784532 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8795475 [details] [diff] [review]
0005-Bug-1294736-5.1-Replace-__always_inline-with-inline-.patch

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

Get that upstream and pull from there.
Attachment #8795475 - Flags: review?(mh+mozilla)
Duplicate of this bug: 1291913
Depends on: 1307570
I've patch 1 out to bug 1307570 to handle the integration of XZ Embedded there.
*moved
Attachment #8795475 - Attachment is obsolete: true
Attachment #8795462 - Attachment is obsolete: true
Blocks: 1307886
This is required if we want to enable CRC64 checks.
Attachment #8784055 - Attachment is obsolete: true
Attachment #8800018 - Flags: review?(mh+mozilla)
Attachment #8800018 - Flags: review?(mh+mozilla) → review+
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0155687dae8
[2.3] Implement XZStream interface for decoding with XZ Embedded. r=glandium,snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/630fc916b83f
[3.2] Detect and decode XZ streams when extracting files. r=glandium,snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae62367e77a9
[5.0] Switch to CRC64 integrity checking. r=glandium
Release Note Request (optional, but appreciated)
[Why is this notable]: terabyte of bandwidth will be saved
[Affects Firefox for Android]: only android
[Suggested wording]: Reduced size of the Firefox for Android app (more than 5MB)
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
better wording from marketing:
"Reduced APK file size by more than 5 MB for faster download and installation"
Bug 1291424 is related and has reduced Fennec startup times by ~15%, maybe combine that into one note?
added to 52beta release notes
You need to log in before you can comment on or make changes to this bug.