854169 - [OS.File] Bindings for lz4

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

We embed library zlib. At the moment, it cannot be used directly from JavaScript, and in particular from workers.

The objective of this bug is to add js-ctypes bindings for zlib.

This can be a mentored bug, but it will be long and difficult and it requires quite a good knowledge of C and JavaScript. You have been warned.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi David, I have a couple of questions:
In order to open a library with js-ctypes, one needs to provide a path to it. I am trying to load a zlibc library from within mozilla-central's code base (modules/zlib), but I'm having difficulty finding examples of how to locate its path.

Would you know if there's a use case like that in the code base somewhere already?

Thanks

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

^

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Here's an IRC log that might be relevant :
http://krijnhoetmer.nl/irc-logs/developers/20130502#l-1690

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Latest update: bug 846410 might make replace this bug. I suggest you wait until some consensus has been reached on bug 846410.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Consensus was reached and the blocker is lifted.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

I have started working on this, so I guess I can as well take that bug.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: WIP 1

I have a first version that is able to perform compression and decompression.
For the moment, decompression only works with an ArrayBuffer. I should make it work also with a pointer.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Exposing the necessary utility function from OS.File

This patch exposes utility normalizeToPointer() in a better place of OS.File.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Build system stuff

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Bindings to lz4

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: 4. OS.File.{read, writeAtomic} support for lz4

Finally, this patch exposes lz4 to OS.File.{read, writeAtomic}.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Bindings to lz4, v2

Added more fine-grained error handling, some documentation, more tests.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: 4. lz4 tests

Ye forgotten tests for part 3.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: 5. OS.File.{read, writeAtomic} support for lz4, v2

Propagating refined error-handling.
I took the opportunity to deprecate |bytes| in |OS.File.read(path, bytes, options)|, as it is rather counter-intuitive now that we have an options object.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 822990 [details] [diff] [review]
4. lz4 tests

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

This looks reasonable.

I think we should include a test in which we try to decompress a pre-existing buffer (i.e. not one generated by compressFileContent) and compare it to its known decompressed contents.  Similarly, we should also compress a known buffer and compare it to a pre-existing buffer.  The buffers should be stored as files or somesuch.  That way, the testsuite ensures that we don't change things without thinking about backwards compatibility.  r=me with that change.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 822989 [details] [diff] [review]
3. Bindings to lz4, v2

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

I'm a little surprised there's no "raw" LZ4 interfaces here, but I guess someone will add them if they seem useful.

::: toolkit/components/workerlz4/lz4.cpp
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#define EXPORT_CTYPES

What is this for?  I don't see any bits in Compression.h that cater to this.

::: toolkit/components/workerlz4/lz4.js
@@ +13,5 @@
> +
> +let SharedAll = require("resource://gre/modules/osfile/osfile_shared_allthreads.jsm");
> +let Internals = require("resource://gre/modules/workers/lz4_internal.js");
> +
> +let MAGIC_NUMBER = (new TextEncoder()).encode("mozLz4a");

Can you define this (raw bytes, etc.) within resorting to dragging in TextEncoder?

Also, can you make this 8 bytes, probably by appending \0, so that content-length and the contents start on 32-bit aligned boundaries?

@@ +17,5 @@
> +let MAGIC_NUMBER = (new TextEncoder()).encode("mozLz4a");
> +
> +let BYTES_IN_SIZE_HEADER = ctypes.uint32_t.size;
> +
> +let HEADER_SIZE = MAGIC_NUMBER.byteLength + BYTES_IN_SIZE_HEADER;

These constants should be, well, |const|.

@@ +112,5 @@
> +      EXPECTED_SIZE_BUFFER_TYPE.ptr).contents;
> +  let expectDecompressedSize = expectDecompressedSizeBuffer[0]
> +   + ( 1 << 8 ) * expectDecompressedSizeBuffer[1]
> +   + ( 1 << 16) * expectDecompressedSizeBuffer[2]
> +   + ( 1 << 24) * expectDecompressedSizeBuffer[3];

How about (edsb = expectDecompressedSizeBuffer):

(edsb[0] + (edsb[1] << 8) + (edsb[2] << 16) + (edsb[3] << 24);

I think this is slightly clearer.

@@ +129,5 @@
> +  // Decompress
> +  let success = Internals.decompress(inputPtr, bytes - HEADER_SIZE,
> +                                     outputBuffer, outputBuffer.byteLength,
> +                                     decompressedBytes.address());
> +  if (!success) {

Is it true that !success iff *decompressedBytes != outputBuffer.byteLength?

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 823050 [details] [diff] [review]
5. OS.File.{read, writeAtomic} support for lz4, v2

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

r=me once I can convince you that |mustBeCompressed| is superfluous. ;)

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +89,5 @@
>       // built-in mechanism for uncaught errors, although this mechanism
>       // may lose interesting information.
> +
> +     // However, rewrite interesting information to take into account
> +     // modules.

Did you mean to include this?

If so, can you split it out to a separate bug or patch?

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +312,5 @@
> + * fields:
> + * - {number} bytes An upper bound to the number of bytes to read.
> + * - {string} compression If "lz4" and if the file is compressed using the lz4
> + *   compression algorithm, decompress the file contents on the fly.
> + * - {bool} mustBeCompressed If |true| and if |compression: "lz4"|

Why would we ever want |compression: "lz4"| and |mustBeCompressed: false|?

Comment 18

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #17)
> ::: toolkit/components/osfile/modules/osfile_shared_front.jsm
> @@ +312,5 @@
> > + * fields:
> > + * - {number} bytes An upper bound to the number of bytes to read.
> > + * - {string} compression If "lz4" and if the file is compressed using the lz4
> > + *   compression algorithm, decompress the file contents on the fly.
> > + * - {bool} mustBeCompressed If |true| and if |compression: "lz4"|
> 
> Why would we ever want |compression: "lz4"| and |mustBeCompressed: false|?

I am thinking of a transition period during which some files may be either uncompressed (if they have been produced by previous versions of Firefox) or compressed (if they have been produced by the latest version). But yes, the more I think about it, the more I realize that we should change the file name when we upgrade to a lzived version.

Consider me convinced :)

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 822815 [details] [diff] [review]
2. Build system stuff

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

::: toolkit/components/workerlz4/moz.build
@@ +13,5 @@
> +    'lz4.js',
> +    'lz4_internal.js',
> +]
> +
> +CPP_SOURCES += [

You've been bitrotted. It's SOURCES, now.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Bindings to lz4, v3

Applied feedback. In particular, I strengthened lz4_decompress for the case in which the number of bytes was lower than the expected number of bytes. I don't think that this can happen, but one never knows.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Bindings to lz4, v4

Same thing, minus typo.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Attachment: 4. lz4 tests, v2

Added reference test.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: 4. lz4 tests, v3

Added missing data file.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Attachment: 5. OS.File.{read, writeAtomic} support for lz4, v3

Removed mustBeCompressed, exception-handling-code-that-is-not-specific-to-this-bug.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 824674 [details] [diff] [review]
5. OS.File.{read, writeAtomic} support for lz4, v3

There are still a few traces of mustBeCompressed here.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: 5. OS.File.{read, writeAtomic} support for lz4, v4

Removed references to now disappeared option mustBeCompressed.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=c914d5f85d06

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Attachment: 4. lz4 tests, v5

Adding forgotten commit message.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Build system stuff, v2

Unbitrotten

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=0f9fcbb1d96e

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Build system stuff, v3

Removing reference to a non-existing module.
Try: https://tbpl.mozilla.org/?tree=Try&rev=27c6f1ddc894

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ccae8b3f0a7d
https://hg.mozilla.org/integration/fx-team/rev/4ede802f8dbb
https://hg.mozilla.org/integration/fx-team/rev/844860e32631
https://hg.mozilla.org/integration/fx-team/rev/12cebd8b5669
https://hg.mozilla.org/integration/fx-team/rev/8898b6c66941

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Android did *not* care for this. Backed out.
https://hg.mozilla.org/integration/fx-team/rev/a8fe75687113

https://tbpl.mozilla.org/php/getParsedLog.php?id=30330422&tree=Fx-Team

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Patch 2 doesn't apply due to glandium's recent SHARED_LIBRARY_LIBS removal. Please post an updated patch (probably with him taking another look at it).

Comment 35

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 828616 [details] [diff] [review]
2. Build system stuff, v3

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

::: toolkit/components/workerlz4/tests/moz.build
@@ +5,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +MODULE = 'test_workerlz4'
> +
> +XPCSHELL_TESTS_MANIFESTS += ['xpcshell/xpcshell.ini']

Does this need its own moz.build?

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Build system stuff, v4

Adapted to the latest changes in our build system.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1b339f171b0a
https://hg.mozilla.org/integration/fx-team/rev/224d0d4df434
https://hg.mozilla.org/integration/fx-team/rev/c0f9bfe37d74
https://hg.mozilla.org/integration/fx-team/rev/cbf7e38a49aa
https://hg.mozilla.org/integration/fx-team/rev/bd7733e5d5cf

Comment 38

[Ryan VanderMeulen [:RyanVM]]

And as predicted by Ms2ger, a bustage follow-up.
https://hg.mozilla.org/integration/fx-team/rev/236a0eba3a8a

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Weird. For some reason, it worked for me, so I didn't think of re-running this on Try.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for various failures. Please give this a full Try run before requesting checkin again. Also please refresh the build system patch while you're at it.
https://hg.mozilla.org/integration/fx-team/rev/2d4fd5a493b1

OSX Marionette bustage:
https://tbpl.mozilla.org/php/getParsedLog.php?id=31324613&tree=Fx-Team

OSX Jetpack timeouts:
https://tbpl.mozilla.org/php/getParsedLog.php?id=31332695&tree=Fx-Team

Android reftest-2 unexpected passes:
https://tbpl.mozilla.org/php/getParsedLog.php?id=31327267&tree=Fx-Team

Comment 41

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5][Back 2-Dec] from comment #38)
> And as predicted by Ms2ger, a bustage follow-up.
> https://hg.mozilla.org/integration/fx-team/rev/236a0eba3a8a

Investigating. I have difficulties believing that any of these issues is related to my patches, though.

Comment 42

[David Teller [:Yoric] - still alive but not very active]

VYV03354, do you have any idea why this could break the Android reftest-2?

Comment 43

[Masatoshi Kimura [:emk]]

Maybe WebFont loading are broken, then both files are rendered with the same system default font? (Variation selectors will be ignored if the font doesn't support.)

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Attachment: 6. Lazy loading

So, the Jetpack problem is actually due to a problem in the Jetpack test runner (bug 952335). Until someone in the Jetpack team takes time to merge the Jetpack test runner and the test runner for the rest of mozilla-central, here is a workaround that simply delays linking to libxul until this is actually needed.

Comment 45

[Nathan Froyd [:froydnj]]

Comment on attachment 8356548 [details] [diff] [review]
6. Lazy loading

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

::: toolkit/components/osfile/modules/osfile_shared_allthreads.jsm
@@ +884,5 @@
>  /**
> + * Representation of a native library.
> + *
> + * The library is opened lazily, during the first call to |open| or
> + * whenever accessing one of its methods for the first time.

Which |open| is this?  You mean the first reference to |.library|?

I don't know that the bit about its methods makes sense with the current code...maybe "when calling one of its methods declared through declareLazyFFI"?

@@ +943,5 @@
> +   * @param {ctypes.abi} abi The abi to use, or |null| for default.
> +   * @param {Type} returnType The type of values returned by the function.
> +   * @param {...Type} argTypes The type of arguments to the function.
> +   */
> +  declareLazyFFI: function(object, field, ...args) {

It would be nice to unify this with the existing |declareLazyFFI| function supported in this module and/or transition everything over to using |Library|.  Followup bug?

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Attachment: 6. Lazy loading, v2

Applied feedback, filed followup as bug 952335.

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=935332f100ab

Comment 48

[David Teller [:Yoric] - still alive but not very active]

Same try after pull: https://tbpl.mozilla.org/?tree=Try&rev=1c8c54dee20f
Looks to me like it's working. Let's try landing this once again.

Comment 49

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Build system stuff, v5

Latest build system stuff.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d043b187d96e
https://hg.mozilla.org/integration/fx-team/rev/1129e0e63c81
https://hg.mozilla.org/integration/fx-team/rev/d0ab9e475844
https://hg.mozilla.org/integration/fx-team/rev/5c4f21043ce1
https://hg.mozilla.org/integration/fx-team/rev/52f2b7583b97
https://hg.mozilla.org/integration/fx-team/rev/a270eb448899

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d043b187d96e
https://hg.mozilla.org/mozilla-central/rev/1129e0e63c81
https://hg.mozilla.org/mozilla-central/rev/d0ab9e475844
https://hg.mozilla.org/mozilla-central/rev/5c4f21043ce1
https://hg.mozilla.org/mozilla-central/rev/52f2b7583b97
https://hg.mozilla.org/mozilla-central/rev/a270eb448899

Comment 53

[Gregory Szorc [:gps]]

Please document this awesome new feature!