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

String.prototype.repeat - resolves #78 #79

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Barbarrosa
Copy link

No description provided.

@elicwhite
Copy link
Contributor

Can you link where you pulled the code from? I can't seem to find it with a quick Google search

@Barbarrosa
Copy link
Author

This is new code based on the ECMA 6 draft spec: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype.repeat

I obtained the meat of the string-concatenating code from this jsperf test: http://jsperf.com/repeat-string-n-times

@tomByrer
Copy link
Contributor

Does this work back to IE8? Have a way to test this out please?
That is an interesting jsPerf test either way!

@Barbarrosa
Copy link
Author

You can simply include the raw file on a page and load it in IE8. The script running will be the updated version, but it will load the normal Spritz CSS, etc.

@Barbarrosa
Copy link
Author

I tested it in Chrome by modifying the bookmarklet.

@tomByrer
Copy link
Contributor

Fast, but let's go faster ^

@Barbarrosa
Copy link
Author

The checking against NaN, Infinity, <0, and Math.floor/Math.abs are necessary for the ECMA 6 compliance. That'll still be faster than the current Spritz method, though.

Also, following the spec means we can expect the same behavior (but faster) when a browser implements the method natively (at least with a mature implementation).

@tomByrer
Copy link
Contributor

Cool, so why strict ES6 compliance really needed? Are the results
different? If num is always an in - range number, is there a check that
needs to run?
On Mar 16, 2014 8:03 PM, "Barbarrosa" [email protected] wrote:

The checking against NaN, Infinity, <0, and Math.floor/Math.abs are
necessary for the ECMA 6 compliance. That'll still be faster than the
current Spritz method, though.

Reply to this email directly or view it on GitHubhttps://github.com//pull/79#issuecomment-37779670
.

@Barbarrosa
Copy link
Author

You may not have received my previous edit in your email. I think this explains it quite well:

"Also, following the spec means we can expect the same behavior (but faster) when a browser implements the method natively (at least with a mature implementation)."

@tomByrer
Copy link
Contributor

we can expect the same behavior (but faster)

I would hope so; there should be alot of redundant mov ops that should be removed, which will also allow the CPU to further optimize the opcodes by interpolation & concurrent execution. (I've done contract work optimizing assembly code by hand.)

But there still ES5 native functions that are faster if you re-write in ES3, like .map, so I will not bet on native being faster. In fact I would bet against it (Chrome 35 result).
EDIT: Preliminary results on Firefox 29 Aurora look good, but I almost think they used my version.

If those specific checks are really needed, sure let them be used. But otherwise, why make slow down peoples' browsers & run down their battery more? Also, it is unlikely majority of mobile browsers will have ES6, so I'd prefer my version as a shim.

@Barbarrosa
Copy link
Author

It's a tradeoff between performance and human efficiency. People who use the function without seeing the source may assume it works like the ECMA 6 spec.

It may befit this project to use a different function altogether and avoid changing the native String object.

@Barbarrosa
Copy link
Author

I would advise against overriding functions that have a spec without also following the spec, lest you reduce interoperability with other JavaScript libraries.

@tomByrer
Copy link
Contributor

use a different function altogether and avoid changing the native String object... that have a spec without also following the spec

Totally agree; I forgot to consider how that function was called. Thanks for pointing that out!

BTW, I think your routine is faster than the current es6-shim. I wish I had time to to a PR for you (funeral & looking for employment). But if you could please make a PR & jsPerf test to prove that your 100% spec version is faster, I'm sure many would benefit from your wisdom.

@Barbarrosa
Copy link
Author

It looks like the es6-shim is actually faster, so perhaps we should consider switching to that algorithm regardless of whether we actually use String.prototype.repeat.

@JannieP
Copy link
Contributor

JannieP commented Mar 18, 2014

I concur with the es6-shim strategy, especially the "repeat" sub function.
var repeat = function(s, times) {
if (times < 1) return '';
if (times % 2) return repeat(s, times - 1) + s;
var half = repeat(s, times / 2);
return half + half;
};

However I also agree with [Barbarrosa] that we should avoid overriding functions
especially since we are only using the overided repeat for the padding.

why not just call this "repeat" sub function as a standalone function?

@tomByrer
Copy link
Contributor

It looks like the es6-shim is actually faster

Do you have a perf for that please? I could not find a perf to prove that; I looked for one & don't have the time to make it.

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.

4 participants