Hacker News Reader

Comment by cogman10 | Hacker News Reader

This is actually a great example of why you SHOULDN'T use left pad.

I've not profiled it, but I'm going to guess that now-a-days this will be faster than the current implementation on npm.

    function leftPad (str, len, ch) {
      str += '';
      len = len - str.length;
      if (len <= 0) return str;
      if (!ch && ch !== 0) ch = ' ';
      ch += '';
      return ch.repeat(len) + str;
    }

Why? because the VM is (very likely) going to do exactly what the cache would have done. It can replace `ch.repeat(len) + str;` with a presized string allocation and a memcpy of ch + str characters.

View on HN

This code checks for ch !== 0, yet the bypass branch afterwards calls .repeat. The Number type in JS has no repeat method.

Static typing would actually fix this kind of coding error.

> This code checks for ch !== 0, yet the Number type in JS has no repeat method.

> Static typing would actually fix this kind of coding error.

That's not a coding error, that's addressing an oddity of JS coercion rules that a less experienced developer could easily have missed.

> if (!ch && ch !== 0) ch = ' '

That code says that if `ch` is falsey and not equal to 0, then set it to a space. The only arguable falsey value that should be excluded here is a literal `false`, but that's not a single character and is fairly ambiguous either way. I'd certainly fall on the side that a literal false should not be converted to `'false'` here.

> ch += '';

The next line converts to to a string by adding it to the empty string.

> return ch.repeat(len) + str;

So by the time it gets to this line we know ch is a string.

Static typing is great, but the bug you claim is there is not actually there.

FYI, I edited the post after the bug was pointed out to eliminate it and bring this method up to parity with leftpad.

What magic would enable that? I don't think the VM is likely to cache a string of 6 spaces, it has no way of knowing that would be a common parameter, and no heuristic to determine that it's likely.

It may specialize or inline, but that's a separate matter.

P.S. I think there's a bug here.

JIT is the magic sauce and this a pretty regular optimization.

    Step 1, inline repeat.
    Step 2, remove the intermediate array allocation
    Step 3, allocated a string array sized for the pad + str
    Step 4, Use one of the many CPU instructions to repeatably copy the padding character and then the `str` into the same array of characters.

None of these optimization would be out of the question for the Jit (and I'd expect them). You don't need the cache at all, it's just a waste. The only thing it saves it creating the intermediate string which is HIGHLY likely to be optimized away with the simple code.

They benchmarked it. Their implementation was faster than ch.repeat: https://github.com/left-pad/left-pad/pull/11#issuecomment-20...

Nowadays there's a native padStart function and the left-pad package is deprecated as a result.

They used a flawed benchmarking methodology.

JITs only optimize hot code which means any benchmark without a warmup is going to measure cold + hot code time.

Further, JITs will optimize away unused results. They aren't using the leftpad results in any meaningful way.

What they are likely measuring is how long it takes for the JIT to optimize the benchmarking framework.

watch: https://vimeo.com/78900556 for how to microbenchmark a JITed language.

Yeah benchmarks can be much more difficult to get right than it might seem at first glance. Good thing they didn't try to write their own benchmarking code, otherwise they might have fallen into those traps you just mentioned.

Luckily, they didn't, and instead pulled in the the `benchmark` library as a development dependency[1]. The author of said library works on V8, and already considered all those problems and much, much, more[2].

[1]: https://github.com/left-pad/left-pad/blob/master/perf/perf.j...

[2]: https://mathiasbynens.be/notes/javascript-benchmarking

I know, I read the benchmark suite code. The benchmark suite itself isn't doing the things I said it isn't doing.

Sure, It's popular. It's also wrong.

Okay, you win. I'm not going to read the entire source of that package just to make a point. Though I do find it strange that the author of that library would write an entire blog post on the topic and then not take his own advice in the implementation of the library he wrote.

Reply on news.ycombinator.com