fixed mem issue when generating uuid

[sava-smith]

Currently, whenever we create an id, we are allocating 600 bytes of memory for each id.
This is because chromium has a bug that does not allow using the '+' operator to allocate memory correctly: https://bugs.chromium.org/p/v8/issues/detail?id=2869

Changing this to a join will fix the size difference:
Concatenate: Shallow Size 40, Retained Size 600
Join: Shallow Size: 64, Retained Size: 64

[broofa]

It'd be nice to have a more concrete recipe for reproducing this. E.g. what diagnostic tools are you using? Specific steps to see the result?

Also, is this causing problems for in-the-wild code, or is this just an academic issue? That Chromium issue has been open for 4+ years. That nobody has complained about it here before has me questioning how important this is.

My apologies for being mulish, but I'm not a fan of patching code for the sake of fixing issues in upstream projects. There are a couple problems with this. One, it increases code complexity. E.g. before accepting your PR, I'll ask that you add a comment explaining why the code is structured the way it is, and referencing the above issue. Two, there are always trade offs. E.g. your PR results in the creation of a [transient] array, thus adding [admittedly trivial] GC perf hits elsewhere.

But, really, this just boils down to the fact it's silly for dozens or hundreds of current and future module owners to have to inject this hack when this should be fixed in Chromium.

[Leaving this open for the time being, since this is a legitimate issue and change request. 'Just not 100% sure it's worth worrying about.]

[Edit: as explained by @jakobkummerow below, this appears to be an expected byproduct of how Chrome and Firefox optimize string concatenation at the expense of cases where many small strings are concatenated. Thus, I'm going to support fixing this here.]

[jakobkummerow]

V8 developer here, just to quickly note that this is unrelated to https://bugs.chromium.org/p/v8/issues/detail?id=2869 (which is about substrings of long strings, not string concatenations created with +).

The reason for the difference in memory consumption is that V8 makes string additions fast by simply creating a "cons string" that refers to the two inputs, rather than copying all the characters. Certain operations later cause such "cons strings" to be "flattened", so depending on what you use the generated UUIDs for, memory consumption may well decrease later.

My opinion is that it is fine to land this patch, as the .join trick makes it easy for any JavaScript engine to recognize what you're doing (i.e. concatenating 20 one-character strings), rather than requiring complicated analysis to figure out what's going on. That said, I also wouldn't worry about it unless you're actually seeing an issue in practice.

[sava-smith]

Sorry that's the bug for a different issue I'm working on. Here is one open bug filled that is about concatenations (in this case they ran out of memory due to the issue).

Our app is using this to create our ids - many at a time, so we are noticing the difference in memory.

If you would like to reproduce this I made this test app: http://plnkr.co/edit/gonu837kTr0IS8uG4SLt?p=preview
I used the chrome developer tools to get the heap snapshot after it created guids using both methods. These were the results I found:

[broofa]

Please add a comment here referencing https://bugs.chromium.org/p/v8/issues/detail?id=3175#c4 as the reason for using Array#join() here, then I'll merge this.

[broofa]

@jakobkummerow Thanks for chiming in.

For the sake of others that may stumble across this, do you happen to know what other operations will cause cons strings to be flattened? (I.e. is Array#join() the most elegant hack here or, for example, is there a way to flatten without needing to create an array?)

[broofa]

For anyone else stumbling across this, it appears that some (but not all) functions that transform a string have the side-effect of flattening the underlying "cons strings". E.g. str.toLowerCase(), str.trim(), str.replace(/^/, ''), and (str + 'x').substr(0, str.length) all appear to work (in v8). str.substr(0) does not, however.

@sava-smith Knowing this, we could do ( ... ).toLowerCase() on the concatenated string instead of [...].join(). three.js is doing this (with toUpperCase instead). But... I'm guessing the join() pattern is a little more efficient(???) so let's stick with that for now.

[broofa]

Please add that comment (noted above)

[sava-smith]

. As far as join vs toLowerCase, I'm not sure if it is more efficient or not. based on memory allocation theyre around the same. I think using tolowercase though might be a little more confusing to anyone else looking at the code.

[broofa]

Thanks! This was actually a fun PR to investigate.

TIL: How to kill Chrome with Strings.

[jakobkummerow]

Re operations that cause flattening: I think the ["a", "b", "c"].join("") pattern nicely expresses intent, so I would suggest to prefer that over relying on a side effect of a completely unrelated operation such as performing a search.

TIL: How to kill Chrome with Strings.

There's nothing special about Strings here, 100 million of just about anything will exceed the heap limit, leading to an out-of-memory crash ;-)

[broofa]

There's nothing special about Strings here

... except that bit about x = y + z producing a value that has references to y and z under the hood. :-)

</snark>