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

Improved performance of Polymer.CaseMap.* functions #3279

Merged

Conversation

nazar-pc
Copy link
Contributor

@nazar-pc nazar-pc commented Jan 7, 2016

Doubled Polymer.CaseMap.dashToCamelCase performance with simplified and once compiled RegExp.
5 times faster Polymer.CaseMap.camelToDashCase using simplified replace part, simplified and once compiled RegExp.

Code is more condenced, but still readable and doesn't use temporary variables.

… and once compiled RegExp.

5 times faster `Polymer.CaseMap.camelToDashCase` using simplified replace part, simplified and once compiled RegExp.
@arthurevans
Copy link

I think this should also fix #3206, which appears to be caused by the camelToDashCase behavior not matching that of dashToCamelCase.

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Jan 8, 2016

Definitely. Should I add test case for that?

@TimvdLippe
Copy link
Contributor

@nazar-pc You can use the test suite I developed for #3213 : https://github.com/Polymer/polymer/pull/3213/files#diff-00b54e7068e156ebcad1fcf0eb6190b6R22

Also the behavior of camelToDashCase is changed in this PR. It will convert CamelCase to -camel-case, which is confusing for the user. The previous version resulted in Camel-case.

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Jan 9, 2016

Even though this behavior might be changed, but I'm not sure how many people are actually using that syntax. Even if you want CamelCase to be mapped to camel-case property, it will be converted back as camelCase anyway, so if we support starting with capital letter - there will be some confusion for users and from implementation perspective.

@TimvdLippe
Copy link
Contributor

Yes I completely agree, just wanted to point it out. Might be a good thing to put in the docs to prevent confusion/mistakes.

nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Feb 1, 2016
@nazar-pc
Copy link
Contributor Author

nazar-pc commented Feb 4, 2016

If this one is selected, I think @TimvdLippe can just modify his PR to contain tests only and @googlebot will be happy with CLA:)

@dfreedm dfreedm self-assigned this Feb 4, 2016
@dfreedm dfreedm added the p0 label Feb 4, 2016
@samccone
Copy link
Contributor

samccone commented Feb 4, 2016

@nazar-pc do you have a test case and performance metrics to back this change up?

Thanks for the PR! :)

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Feb 4, 2016

I've just took attribute with few sections and executed it something like 100k times and measured how long it took.
You can try something like this in console:

var dash = 'relatively-long-tag-sample';
var camel = 'relativelyLongTagSample';
  var before = {
    _caseMap: {},
    dashToCamelCase: function(dash) {
      var mapped = before._caseMap[dash];
      if (mapped) {
        return mapped;
      }
      // TODO(sjmiles): is rejection test actually helping perf?
      if (dash.indexOf('-') < 0) {
        return before._caseMap[dash] = dash;
      }
      return before._caseMap[dash] = dash.replace(/-([a-z])/g, 
        function(m) {
          return m[1].toUpperCase(); 
        }
      );
    },
    camelToDashCase: function(camel) {
      var mapped = before._caseMap[camel];
      if (mapped) {
        return mapped;
      }
      return before._caseMap[camel] = camel.replace(/([a-z][A-Z])/g, 
        function (g) { 
          return g[0] + '-' + g[1].toLowerCase() 
        }
      );
    }
  };
  var after = {
    _caseMap: {},
    _rx: {
      dashToCamel: /-[a-z]/g,
      camelToDash: /([A-Z])/g
    },
    dashToCamelCase: function(dash) {
      return this._caseMap[dash] || (
        this._caseMap[dash] = dash.indexOf('-') < 0 ? dash : dash.replace(this._rx.dashToCamel,
          function(m) {
            return m[1].toUpperCase();
          }
        )
      );
    },
    camelToDashCase: function(camel) {
      return this._caseMap[camel] || (
        this._caseMap[camel] = camel.replace(this._rx.camelToDash, '-$1').toLowerCase()
      );
    }
  };
console.time('before-1');
for (var i = 0; i < 100000; ++i) {
  before.dashToCamelCase(dash + i);
}
console.timeEnd('before-1');
console.time('before-2');
for (var i = 0; i < 100000; ++i) {
  before.camelToDashCase(camel + i);
}
console.timeEnd('before-2');

console.time('after-1');
for (var i = 0; i < 100000; ++i) {
  after.dashToCamelCase(dash + i);
}
console.timeEnd('after-1');
console.time('after-2');
for (var i = 0; i < 100000; ++i) {
  after.camelToDashCase(camel + i);
}
console.timeEnd('after-2');

Output will be like following:

before-1: 1406.400ms
before-2: 1391.004ms
after-1: 342.465ms
after-2: 344.330ms

You can play with attribute length, i is added to avoid caching.

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Feb 4, 2016

Test case for the bug that was fixed accidentally can be taken from #3213 as is.

@dfreedm
Copy link
Member

dfreedm commented Feb 6, 2016

LGTM

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

Successfully merging this pull request may close these issues.

6 participants