Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ES6 Map/Set when possible (2x speedup in compress). #47

Open
ChALkeR opened this issue Mar 14, 2015 · 14 comments
Open

Use ES6 Map/Set when possible (2x speedup in compress). #47

ChALkeR opened this issue Mar 14, 2015 · 14 comments

Comments

@ChALkeR
Copy link
Contributor

ChALkeR commented Mar 14, 2015

Using ES6 Map for context_dictionary and ES6 Set (or Map) for context_dictionaryToCreate instead of a plain Object speeds up the compression twice in v8.

See #46 (comment) for test results.

Map and Set are supported in current Chrome and Firefox versions.
IE11 has partial support for them, but that should be enough: see «Map → basic functionality» here: https://kangax.github.io/compat-table/es6/

I haven't tested the speed in Firefox and IE11, but it shouldn't be worse than using an Object.

Supporting old browsers would require some kind of feature detection.

@ChALkeR ChALkeR changed the title Use ES6 Map/Set when possible. Use ES6 Map/Set when possible (2x speedup in compress). Mar 14, 2015
@pieroxy
Copy link
Owner

pieroxy commented Mar 15, 2015

Thanks for the idea and the tests. I've always wanted lz-stringto be compliant with every javascript engine, hence the avoidance of new JavaScript stuff. I've also done a few tests to work with arrays instead of Strings but I probably didn't push them far enough.

If you have the code with ES6 Map/Set, can you push it through a pull request ? I can probably have a look into it this week. Detecting the feature shouldn't be that hard.

ALSO: Can you take a minute and pass the jsperf of the latest pre-release (https://github.com/pieroxy/lz-string/releases) with all the IEs you have at hand? I can't decide myself to release it (faster on FF/Chrome, slower on IE) but I lack data on IE and my tests were performed in a VM which is probable messing with performances.

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Mar 15, 2015

I don't have any IE versions, sorry.
What about #46? Should I make a pull request for that? That one is more severe than this.

@pieroxy
Copy link
Owner

pieroxy commented Mar 15, 2015

I've had my share of surprises on jsperf with code that would OBVIOUSLY be faster but was slower on some browsers for reasons I couldn't be bothered to investigate.

I'd start with doing a couple of jsperf with this one as it looks like it's going to be beneficial for #46 as well. We'll move forward if tests are a clear win on all platform supported. Moving forward meaning:

  • Feature detection
  • implement the new version if feature is there while keeping the actual for old browsers
  • We need to be careful not to break anything in node.js as well.

NOTE: I've just released v1.4.0 which clean up the wrapper methods. If you can rebase on top of that it would be great.

Either you do the jsperf yourself or you push a pull request and I'll deal with the jsperf.

And thanks for the hard work !

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Mar 15, 2015

This one won't help #46.
The problem with #46 is the memory usage, jsperf doesn't measure that.
String concatenation is bad, it's not how strings are supposed to be used. It easily consumes two orders of magnitude more memory than push/join.

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Mar 15, 2015

I made a pull request for #46.

@pieroxy
Copy link
Owner

pieroxy commented Mar 23, 2015

I'll start working on that one starting next week. We'll obviously start with a jsperf ;-)

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Mar 23, 2015

With this issue, of course =).

@pieroxy
Copy link
Owner

pieroxy commented May 10, 2015

Allright, it took longer than expected, as usual. Here is a glimpse of what could be LZString 1.5:

http://jsperf.com/lz-string-1-4-3-vs-1-5

Can you take it with as many devices as possible?

So far, unsurprisingly, old browsers take a hit, new ones are happy. A few exceptions:

  • Android's default browser is slower on v1.5 - doesn't surprise me from this piece of shit
  • IE even 11 is also slower on v1.5
  • Safari on MacOSX is also somewhat slower with the new version

FYI, the code: c8ac062

@bradvogel
Copy link

@pieroxy Ping on this. Looking forward to the big perf fix! What's the status on 1.5? Do you just need people to test it? How can we help?

@pieroxy
Copy link
Owner

pieroxy commented Oct 25, 2015

Ah yes, I've been lazy (and busy). There are a couple of things I need to do on LZ-String and this is one of them.

The version 1.5 wasn't all that impressive. The speedup was impressive but the loss due to encapsulation on the old / unsupported browsers was too high.

I'll redo it shortly in another way.

@ronag
Copy link

ronag commented Mar 17, 2017

@pieroxy: ping?

@pieroxy
Copy link
Owner

pieroxy commented Mar 23, 2017

Yup, I'll get around to it shortly now that jsperf is up again.

@ryancdotorg
Copy link

Here's a wrapper around Map that implements just enough functionality for lz-string's purposes when Map isn't available.

var BasicMap = (function () {
    function BasicMap() {
        if (typeof Map === 'function') {
            return (new Map());
        } else {
            this.dict = {};
            return this;
        }
    }
    BasicMap.prototype.set = function (key, val) {
        this.dict[key] = val;
        return this;
    };
    BasicMap.prototype.get = function (key) {
        return Object.prototype.hasOwnProperty.call(this.dict, key) ? this.dict[key] : undefined;
    };
    return BasicMap;
}());

@jimmykane
Copy link

ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants