803377 - Add function to compress string data

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Implement string component with gzip encoding, v1

AFAICT there is no generic JS module for performing string encodings. I constantly find myself having to muck about with nsIStringConverterService and friends. I think it would be really nice if all this XPCOM voodoo were hidden from view and we could easily perform common string encodings.

The attached patch adds a new "string" component to toolkit. In it, we have a single file, StringEncoding.jsm. In that, we have the StringEncoding object which exposes an API for performing gzip compression.

Other encodings can be added to this file in the future. Other features can be added to our string library as well. Now we have a place for them to call home.

Gavin gets review. Mossop gets f? for approving new toolkit additions (since he is toolkit module owner).

I'm not sure what to do about the test. I don't want to put in data I obtained by calling the API itself. And, I was unable to figure out a trusted input and output pair to verify with. I tried Python's zlib.compress and it returned something different. I /think/ our built-in gzip encoder is writing out extra header data (perhaps a file header?) but I really have no clue. I'm also not an expert with nsIStringConverterService and friends, so I might have done something horribly wrong in XPCOM land.

Comment 1

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

Oh, you meant nsIStreamConverterService (not nsIStringConverterService) - that confused me for a bit. The idea of consolidating this code seems reasonable, but I only see one other potential caller of this specific method (in TelemetryPing.js). There are other test callers of asyncConvertData with string streams, but they mostly use different to/from parameters.

This seems like something that could live in a toolkit/modules, were we to create one (ala browser/modules). A whole new components directory to contain just a single module and its test seems like overkill.

The proliferation of small, very specific JS modules is a bit problematic because the fixed overhead of a JS module is rather high at the moment (see e.g. bug 773845 and bug 798491), but that's a problem we probably should be addressing separately either way, so I guess we shouldn't worry about it too much here. Worth being aware of, though.

Comment 2

[Gregory Szorc [:gps]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> Oh, you meant nsIStreamConverterService (not nsIStringConverterService) -
> that confused me for a bit. The idea of consolidating this code seems
> reasonable, but I only see one other potential caller of this specific
> method (in TelemetryPing.js). There are other test callers of
> asyncConvertData with string streams, but they mostly use different to/from
> parameters.

I have lots of code in services/common/utils.js and services/sync/modules/util.js that could be moved to this module and related string modules.

> This seems like something that could live in a toolkit/modules, were we to
> create one (ala browser/modules). A whole new components directory to
> contain just a single module and its test seems like overkill.

I will create toolkit/modules then.

> The proliferation of small, very specific JS modules is a bit problematic
> because the fixed overhead of a JS module is rather high at the moment (see
> e.g. bug 773845 and bug 798491), but that's a problem we probably should be
> addressing separately either way, so I guess we shouldn't worry about it too
> much here. Worth being aware of, though.

I think I'm going to bury my head in the sand here as being discouraged from writing small, loosely coupled and reusable modules is so wrong on so many levels. That being said, concatenating files together into a single module is doable with a little build system magic and maybe some minor changes to how we do modules in Gecko. Hopefully that's never my problem ;)

Comment 3

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

I'm not discouraging you from doing it, I'm making you aware of some of our current platform limitations that you should consider, as part of the cost/benefit analysis exercise that is software development :)

Comment 4

[Gregory Szorc [:gps]]

Normally I'd say the benefits of a line of code written in 1 place instead of N outweighs a slight memory increase due to module proliferation. But B2G. Dammit.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 673043 [details] [diff] [review]
Implement string component with gzip encoding, v1

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

::: toolkit/components/string/tests/unit/test_gzip_encoding.js
@@ +18,5 @@
> +
> +  do_check_true(result.length > 0);
> +
> +  // We should probably verify results against an expected value. However,
> +  // how does one obtain an expected value without calling the API itself?

FWIW, for the tests I added when fixing gzip bugs in the platform code, I used the result of gzip itself.  IIRC, there were a few fiddly details that required hex-editing the gzip output so our gzip implementation would be happy with it, all in non-required fields in the gzip format.

...but maybe you think doing that is scarier than not having any tests at all. ;)

Comment 6

[Gregory Szorc [:gps]]

Nathan: Can you just refer me to a known input and output that I can hard-code into the test? I'm basically looking for a smoke test.

Comment 7

[Gregory Szorc [:gps]]

Ping regarding comment #6.

Comment 8

[Nathan Froyd [:froydnj]]

modules/libjar/zipwriter/test/unit/test_bug717061.js
modules/libjar/zipwriter/test/unit/data/test_bug717061.html
modules/libjar/zipwriter/test/unit/data/test_bug717061.gz

Are the bits I was talking about in comment 5.  See:

http://mxr.mozilla.org/mozilla-central/source/modules/libjar/zipwriter/test/unit/test_bug717061.js#51

for some of the fiddly bits.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 673043 [details] [diff] [review]
Implement string component with gzip encoding, v1

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

As said elsewhere, let's pull the trigger and create toolkit/modules and put this in there.

::: toolkit/components/string/StringEncoding.jsm
@@ +10,5 @@
> +
> +/**
> + * Provides utility functions for performing encoding operations on strings.
> + *
> + * Every find yourself encoding a string to another type or format? This

s/Every/Ever/

@@ +14,5 @@
> + * Every find yourself encoding a string to another type or format? This
> + * object is for you! Simply add your encoder and/or decoder to this and
> + * share it with the world.
> + */
> +let StringEncoding = {

Is this actually appropriate? To me string encoding is all about character encodings (UTF8, UTF16, etc.). Gzip seems different to that. Maybe StringConverter is better?

@@ +24,5 @@
> +   * @param onComplete
> +   *        (function) Callback invoked with compressed output as a string
> +   *        parameter.
> +   */
> +  gzipEncodeToString: function gzipEncodeToString(s) {

This is a really confusing name. How about convertToGzip?

I'm concerned that this is a synchronous method and people might try to use it for large chunks of data. What are the performance characteristics of this?

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 673043 [details] [diff] [review]
Implement string component with gzip encoding, v1

I rewrote a lot of this patch locally. Clearing review flag.

Comment 11

[Gregory Szorc [:gps]]

(In reply to Dave Townsend (:Mossop) from comment #9)
> Comment on attachment 673043 [details] [diff] [review]
> Implement string component with gzip encoding, v1
> 
> Review of attachment 673043 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As said elsewhere, let's pull the trigger and create toolkit/modules and put
> this in there.

Already done in my local patch (to be uploaded).


> @@ +14,5 @@
> > + * Every find yourself encoding a string to another type or format? This
> > + * object is for you! Simply add your encoder and/or decoder to this and
> > + * share it with the world.
> > + */
> > +let StringEncoding = {
> 
> Is this actually appropriate? To me string encoding is all about character
> encodings (UTF8, UTF16, etc.). Gzip seems different to that. Maybe
> StringConverter is better?

Sure.

> @@ +24,5 @@
> > +   * @param onComplete
> > +   *        (function) Callback invoked with compressed output as a string
> > +   *        parameter.
> > +   */
> > +  gzipEncodeToString: function gzipEncodeToString(s) {
> 
> This is a really confusing name. How about convertToGzip?

If we move this to a StringConverter type, then that would definitely make sense.

> I'm concerned that this is a synchronous method and people might try to use
> it for large chunks of data. What are the performance characteristics of
> this?

If you start with a String, you have all the data available. So, making this async would require splitting data manually, event loop spinning, etc. I'm not convinced that's a good idea.

I think a better idea is to just not use these functions if you have large values: use streams instead.

Comment 12

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #11)
> (In reply to Dave Townsend (:Mossop) from comment #9)
> > Comment on attachment 673043 [details] [diff] [review]
> > Implement string component with gzip encoding, v1
> > 
> > Review of attachment 673043 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > As said elsewhere, let's pull the trigger and create toolkit/modules and put
> > this in there.
> 
> Already done in my local patch (to be uploaded).
> 
> 
> > @@ +14,5 @@
> > > + * Every find yourself encoding a string to another type or format? This
> > > + * object is for you! Simply add your encoder and/or decoder to this and
> > > + * share it with the world.
> > > + */
> > > +let StringEncoding = {
> > 
> > Is this actually appropriate? To me string encoding is all about character
> > encodings (UTF8, UTF16, etc.). Gzip seems different to that. Maybe
> > StringConverter is better?
> 
> Sure.
> 
> > @@ +24,5 @@
> > > +   * @param onComplete
> > > +   *        (function) Callback invoked with compressed output as a string
> > > +   *        parameter.
> > > +   */
> > > +  gzipEncodeToString: function gzipEncodeToString(s) {
> > 
> > This is a really confusing name. How about convertToGzip?
> 
> If we move this to a StringConverter type, then that would definitely make
> sense.
> 
> > I'm concerned that this is a synchronous method and people might try to use
> > it for large chunks of data. What are the performance characteristics of
> > this?
> 
> If you start with a String, you have all the data available. So, making this
> async would require splitting data manually, event loop spinning, etc. I'm
> not convinced that's a good idea.

It shouldn't need any event loop spinning, you'd just make a stream out of the string and async feed it to the stream converter and call a callback when done.

> I think a better idea is to just not use these functions if you have large
> values: use streams instead.

I think that's a great idea too, but it's difficult to enforce that on people so providing a way to do it seems like a potential footgun for the future.

Comment 13

[Gregory Szorc [:gps]]

Changing bug to reflect its original purpose and moving out of Toolkit. Hopefully I planted a seed and Toolkit will add something similar one day.

Comment 14

[Gregory Szorc [:gps]]

Attachment: Add CommonUtils.compressStringToString, v1

I wish the test were more robust. Then again, I'm not really testing the core stream encoding code, am I?

Comment 15

[Gregory Szorc [:gps]]

Attachment: Add CommonUtils.convertString, v1

More generic solution to avoid XPCOM masturbation. I'm sure this function has already been implemented somewhere in the tree. But, I can't find it.

Comment 16

[Gregory Szorc [:gps]]

Attachment: Add CommonUtils.convertString, v2

Forgot to qref.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 677891 [details] [diff] [review]
Add CommonUtils.convertString, v2

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

Let's get a UTF-8 test first. Even if the breakage is elsewhere in the codebase, I don't want to ship a handy-dandy utility that makes it easier to shoot oneself in the foot.

::: services/common/tests/unit/test_utils_convert_string.js
@@ +10,5 @@
> +  run_next_test();
> +}
> +
> +add_test(function test_compress_string() {
> +  const INPUT = "hello";

Please put non-ASCII characters into this test string. I opted for "pïgéons1" for my Android Sync text encoding test, but I encourage you to also pass the Turkey Test :)

E.g.,

"Árvíztűrő tükörfúrógép いろはにほへとちりぬるを Pijamalı hasta, yağız şoföre çabucak güvendi."

::: services/common/utils.js
@@ +611,5 @@
> +   */
> +  convertString: function convertString(s, source, dest) {
> +    let is = Cc["@mozilla.org/io/string-input-stream;1"]
> +               .createInstance(Ci.nsIStringInputStream);
> +    is.setData(s, s.length);

Check for !s.

@@ +614,5 @@
> +               .createInstance(Ci.nsIStringInputStream);
> +    is.setData(s, s.length);
> +
> +    let scs = Cc["@mozilla.org/streamConverters;1"]
> +                .getService(Ci.nsIStreamConverterService);

Is this something we ought to be wrapping in a lazy getter? We wrap our scriptableunicodeconverter…

Comment 18

[Gregory Szorc [:gps]]

Attachment: Add CommonUtils.convertString, v3

Additional Unicode string test did expose an error. Turns out JS String -> IDL's "string" throws away data in the high byte octet from JS strings. I documented this limitation in the API and made the test convert the input to an "octet string" before feeding into convertString(). That change made things work.

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 678478 [details] [diff] [review]
Add CommonUtils.convertString, v3

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

Hell yeah test coverage.

Comment 20

[Gregory Szorc [:gps]]

https://hg.mozilla.org/projects/larch/rev/f2135051882c

Comment 21

[Dave Townsend [:mossop]]

This is going to get superreview before landing in m-c right?

Comment 22

[Gregory Szorc [:gps]]

(In reply to Dave Townsend (:Mossop) from comment #21)
> This is going to get superreview before landing in m-c right?

Why would this need super review?

Comment 23

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #22)
> (In reply to Dave Townsend (:Mossop) from comment #21)
> > This is going to get superreview before landing in m-c right?
> 
> Why would this need super review?

It adds an API

Comment 24

[Mike Connor [:mconnor]]

Because it's an API, under the SR policy: http://www.mozilla.org/hacking/reviewers.html

Feel free to tag me for SR.

Comment 25

[Richard Newman [:rnewman]]

Comment on attachment 678478 [details] [diff] [review]
Add CommonUtils.convertString, v3

SR flag has gone missing…?

Comment 26

[Richard Newman [:rnewman]]

Comment on attachment 678478 [details] [diff] [review]
Add CommonUtils.convertString, v3

Bulk-setting approval flags for FHR landing for FxOS ADU ping (Bug 788894).

Comment 27

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48f311b7d355

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48f311b7d355

Comment 29

[Alex Keybl [:akeybl]]

Comment on attachment 678478 [details] [diff] [review]
Add CommonUtils.convertString, v3

FHR for B2G ADU ping, won't be build/enabled for Mobile/Desktop.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2445b602fe37
https://hg.mozilla.org/releases/mozilla-beta/rev/6a2ef4747f2a