513363 - share/improve n-way string concat (when elements are not already strings)

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Namely, str_concat could use the Vector/js_ValueToCharBuffer combo to reap similar benefits as bug 200505.

Comment 1

[Paul Biggar]

Is this bug still valid, and still a [good first bug]?

Comment 2

[Nicholas Nethercote [inactive]]

String concatenation had massive changes (improvements) in Firefox 4, thanks to the use of ropes.  I seriously doubt this bug is still valid.  I'll close it (and remove the [good first bug]), please reopen if you disagree.

Comment 3

[Luke Wagner [:luke]]

Comment 0 was referring to the use of js_ValueToCharBuffer (now js::ValueToStringBuffer) which avoids creating a ton of temporary strings when the elements of the array are not strings (bug 200505 showed a massive speedup on just such a use case in Peacekeeper).

Of course, with ropes, we now have a dilemma: if our array is full of strings, it would probably be more efficient to use js_ConcatStrings since that will create ropes and avoid copying the char buffers; if the array is full of non-strings, we would surely want to use ValueToStringBuffer.  The "probably" in the all-strings case is because, if we have an array full of really-short strings, the cost of copying the char buffers is smaller than allocating and initializing the rope headers.  So, really, we want a hybrid algorithm.

Of course, as I say this, I realize that we should do the same for Array.prototype.join and factor out a shared algorithm.  Getting a 2x improvement is a good feeling, so keeping [good first bug].

Going further: bug 648198 wants to add an n-ary rope node; using that here would further improve our join/concat perf when all the elements were strings.

Comment 4

[Luke Wagner [:luke]]

It would probably be good to instrument a browser and measure to see how often concat is passed all-strings vs. non-strings vs. mix-strings-and-non-strings.

Comment 5

[Victor Carlquist]

Hi, I would like to work on this bug. As this would be my first bug I would like additional guidance. Can I contact over IRC?

Comment 6

[Luke Wagner [:luke]]

Hmm, it's been a while since 2011 and a fair amount has changed.  In particular, nowadays, a string optimization like this would go in the JIT (so it could take better advantage of ssa and type info), but the JIT isn't the easiest thing to dive into if you're not already familiar with compilers.

Thinking more about this, it's not super-clear that optimizing (non-string + string) to eagerly concatenate is a good idea.  In a long chain (string + non-string + string + non-string + string...), it may actually be better to do what we do now and build a big rope; even that does create some temporary string garbage for the converted non-strings.

So I'm afraid I don't have any good first steps on this bug other than to point you at our list of general VM string optimizations:
  http://mxr.mozilla.org/mozilla-central/source/js/src/vm/String.h#42
and where string concatenation starts in the IonMonkey pipeline:
  http://mxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#3828

Sorry if this wasn't a very [good first bug]; I'll remove [good first bug] to avoid false advertising, but feel free to investigate further.  If you don't, I'll probably just close the bug as njn suggested above; we can always open a new one if we find a good concrete use case.

Comment 7

[Victor Carlquist]

Ok, Thanks Luke =)

Comment 8

[Luke Wagner [:luke]]

np!  If you're looking for a first bug, maybe you could check out bug 688219.  It's no longer important for the Peacekeeper benchmark, but it's reported to help jQuery.

Comment 9

[Victor Carlquist]

I'll work on this bug,