Closed Bug 854169 Opened 7 years ago Closed 6 years ago

[OS.File] Bindings for lz4

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: Yoric, Assigned: Yoric)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(6 files, 15 obsolete files)

6.67 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
10.20 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
9.24 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
7.19 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
5.44 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
1.92 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → yura.zenevich
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
^
Flags: needinfo?(dteller)
Here's an IRC log that might be relevant :
http://krijnhoetmer.nl/irc-logs/developers/20130502#l-1690
Latest update: bug 846410 might make replace this bug. I suggest you wait until some consensus has been reached on bug 846410.
Flags: needinfo?(dteller)
Summary: [OS.File] Bindings for zlib → [OS.File] Bindings for lz4
Consensus was reached and the blocker is lifted.
I have started working on this, so I guess I can as well take that bug.
Assignee: yura.zenevich → dteller
Attached patch WIP 1 (obsolete) — Splinter Review
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.
This patch exposes utility normalizeToPointer() in a better place of OS.File.
Attachment #822603 - Attachment is obsolete: true
Attachment #822813 - Flags: review?(nfroyd)
Attached patch 2. Build system stuff (obsolete) — Splinter Review
Attachment #822815 - Flags: review?(mh+mozilla)
Attached patch 3. Bindings to lz4 (obsolete) — Splinter Review
Attachment #822816 - Flags: review?(nfroyd)
Finally, this patch exposes lz4 to OS.File.{read, writeAtomic}.
Attachment #822817 - Flags: review?(nfroyd)
Attached patch 3. Bindings to lz4, v2 (obsolete) — Splinter Review
Added more fine-grained error handling, some documentation, more tests.
Attachment #822816 - Attachment is obsolete: true
Attachment #822816 - Flags: review?(nfroyd)
Attachment #822989 - Flags: review?(nfroyd)
Attached patch 4. lz4 tests (obsolete) — Splinter Review
Ye forgotten tests for part 3.
Attachment #822990 - Flags: review?(nfroyd)
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.
Attachment #822817 - Attachment is obsolete: true
Attachment #822817 - Flags: review?(nfroyd)
Attachment #823050 - Flags: review?(nfroyd)
Attachment #822813 - Flags: review?(nfroyd) → review+
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.
Attachment #822990 - Flags: review?(nfroyd) → review+
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?
Attachment #822989 - Flags: review?(nfroyd) → review+
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|?
Attachment #823050 - Flags: review?(nfroyd) → review+
(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 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.
Attachment #822815 - Flags: review?(mh+mozilla) → review+
Attached patch 3. Bindings to lz4, v3 (obsolete) — Splinter Review
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.
Attachment #822989 - Attachment is obsolete: true
Attachment #824633 - Flags: review+
Same thing, minus typo.
Attachment #824633 - Attachment is obsolete: true
Attachment #824670 - Flags: review+
Attached patch 4. lz4 tests, v2 (obsolete) — Splinter Review
Added reference test.
Attachment #822990 - Attachment is obsolete: true
Attachment #824672 - Flags: review+
Attached patch 4. lz4 tests, v3 (obsolete) — Splinter Review
Added missing data file.
Attachment #824672 - Attachment is obsolete: true
Attachment #824673 - Flags: review+
Removed mustBeCompressed, exception-handling-code-that-is-not-specific-to-this-bug.
Attachment #823050 - Attachment is obsolete: true
Attachment #824674 - Flags: review+
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.
Removed references to now disappeared option mustBeCompressed.
Attachment #824674 - Attachment is obsolete: true
Attachment #827998 - Flags: review+
Attached patch 4. lz4 tests, v5Splinter Review
Adding forgotten commit message.
Attachment #824673 - Attachment is obsolete: true
Attachment #828009 - Flags: review+
Attached patch 2. Build system stuff, v2 (obsolete) — Splinter Review
Unbitrotten
Attachment #822815 - Attachment is obsolete: true
Attachment #828010 - Flags: review+
Attached patch 2. Build system stuff, v3 (obsolete) — Splinter Review
Removing reference to a non-existing module.
Try: https://tbpl.mozilla.org/?tree=Try&rev=27c6f1ddc894
Attachment #828010 - Attachment is obsolete: true
Attachment #828616 - Flags: review+
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
Flags: in-testsuite+
Whiteboard: [mentor=Yoric][lang=js][lang=c][lang=c++][not for the faint of heart][fixed-in-fx-team] → [mentor=Yoric][lang=js][lang=c][lang=c++][not for the faint of heart]
Depends on: 940232
No longer depends on: 940232
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).
Keywords: checkin-needed
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?
Attached patch 2. Build system stuff, v4 (obsolete) — Splinter Review
Adapted to the latest changes in our build system.
Attachment #828616 - Attachment is obsolete: true
Attachment #8339306 - Flags: review?(mh+mozilla)
Attachment #8339306 - Flags: review?(mh+mozilla) → review+
And as predicted by Ms2ger, a bustage follow-up.
https://hg.mozilla.org/integration/fx-team/rev/236a0eba3a8a
Weird. For some reason, it worked for me, so I didn't think of re-running this on Try.
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
Flags: in-testsuite+
Whiteboard: [mentor=Yoric][lang=js][lang=c][lang=c++][not for the faint of heart][fixed-in-fx-team] → [mentor=Yoric][lang=js][lang=c][lang=c++][not for the faint of heart]
(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.
VYV03354, do you have any idea why this could break the Android reftest-2?
Flags: needinfo?(VYV03354)
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.)
Flags: needinfo?(VYV03354)
Attached patch 6. Lazy loading (obsolete) — Splinter Review
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.
Attachment #8356548 - Flags: review?(nfroyd)
Whiteboard: [mentor=Yoric][lang=js][lang=c][lang=c++][not for the faint of heart]
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?
Attachment #8356548 - Flags: review?(nfroyd) → review+
Applied feedback, filed followup as bug 952335.
Attachment #8356548 - Attachment is obsolete: true
Attachment #8357079 - Flags: review+
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.
Keywords: checkin-needed
Latest build system stuff.
Attachment #8339306 - Attachment is obsolete: true
Attachment #8358368 - Flags: review+
Duplicate of this bug: 846410
Please document this awesome new feature!
Keywords: dev-doc-needed
Blocks: 1055317
You need to log in before you can comment on or make changes to this bug.