Skip to content

Conversation

@Rycochet
Copy link
Collaborator

Remove the redundant temporary buffer, and push the output character directly into the output buffer. This saves memory and is quicker as it doesn't need to iterate twice.

(Minified version not updated as edited on GitHub directly)

Remove the redundant temporary buffer, and push the output character directly into the output buffer. This saves memory and is quicker as it doesn't need to iterate twice.
@pieroxy
Copy link
Owner

pieroxy commented Sep 26, 2016

Sorry for the delay. I will setup a small jsperf.com test with this vs the head of the repo to make sure it works on all browsers. When that's done a we have a little data I'll merge it.

Thanks for the effort.

@pieroxy
Copy link
Owner

pieroxy commented Sep 27, 2016

But jsperf.com is down :-/

@Rycochet
Copy link
Collaborator Author

It has been for most of this year - makes life a lot harder for testing stuff :-/

You can manually test in multiple browsers - but the code can be eyeballed as being identical in function, just not using a temporary buffer.

@lishid
Copy link

lishid commented Oct 10, 2016

@pieroxy While we're on this topic, do you know why compressToUint8Array and decompressFromUint8Array don't use _compress and _decompress directly, and instead adds an extra conversion step using a UCS-2 string?

For example, the code sample below is fully compatible with the current implementation:
(It makes decompressFromUint8Array significantly simpler)

  compressToUint8Array: function (input) {
    var compressed = LZString._compress(input, 8, function (a) {
      return f(a);
    });

    // For backwards compatibility with old format using UCS-2
    if (compressed.length % 2 !== 0) {
      compressed = compressed + f(0);
    }

    var buf = new Uint8Array(compressed.length);

    for (var i = 0, TotalLen = compressed.length; i < TotalLen; i++) {
      buf[i] = compressed.charCodeAt(i);
    }

    return buf;
  },

  decompressFromUint8Array: function (compressed) {
    if (compressed === null || compressed === undefined) {
      return LZString.decompress(compressed);
    }
    else {
      if (compressed.length == 0) return null;
      return LZString._decompress(compressed.length, 128, function (index) {
        return compressed[index];
      });
    }
  },

If you think this new implementation is worth a shot, I could send in a PR.

@rbndelrio
Copy link

@pieroxy worth noting that jsperf.com is back online!

@pieroxy
Copy link
Owner

pieroxy commented Mar 11, 2017

Right, thanks!

@JobLeonard
Copy link
Collaborator

JobLeonard commented Jun 25, 2017

@lishid: Actually, this can be optimised even further for compressToUint8Array: right now _compress (and by extension compress) returns a string. However, the intermediate code relies on an array of chars that is converted to a string near the end.

So if we make a new function, say compressToArray, which contains all the code of _compress except that the last line of:

    return context_data.join('');

becomes:

    return context_data;

And then rewrite _compress as:

  _compress: function (uncompressed, bitsPerChar, getCharFromInt) {
    return compressToArray(uncompressed, bitsPerChar, getCharFromInt).join('')
  }

(which makes it backwards compatible with existing compressors at the cost of a single extra function call), then we can change your code to:

  compressToUint8Array: function (input) {
    var compressed = LZString.compressToArray(input, 8, function (a) {
      return f(a);
    });

    // For backwards compatibility with old format using UCS-2
    if (compressed.length % 2 !== 0) {
      compressed.push(f(0));
    }

    // we could even replace this with Uint8Array.from(compressed)
    // if not for backwards compatibility
    var buf = new Uint8Array(compressed.length);

    // this can theoretically be sped up even further with loop-unrolling:
    // http://jsbench.github.io/#80b894adfe3e635ef03e7f6ebc55d086
    for (var i = 0, TotalLen = compressed.length; i < TotalLen; i++) {
      buf[i] = compressed[i];
    }

    return buf;
  },

This saves use the creation of a string, and array lookup is probably faster than charAt(i) as well

@Rycochet
Copy link
Collaborator Author

@JobLeonard I didn't even look at it that deeply - nice - if you make that into a PR this one can be closed :-D

@JobLeonard
Copy link
Collaborator

@Rycochet: look a bit closer: my optimisation is for compression, yours for decompression ;).

The mistake is understandable though, they're doing similar things. You know what, tomorrow I'll try to combine all suggestions in this thread and push a PR!

@Rycochet
Copy link
Collaborator Author

@JobLeonard Doh, apparently I didn't look back at my change either lol - go for it ;-)

@Rycochet
Copy link
Collaborator Author

Closing in preference of #98 - includes this and other code in these comments.

@Rycochet Rycochet closed this Jun 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants