608776 - use a per-compartment ballast to avoid malloc in js_ConcatStrings

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Currently, we eagerly acquire buffer space in js_ConcatStrings -- even though writing to this buffer is done lazily -- since we want flatten() to be infallible.  Ripping out all eager malloc (infallible-malloc'ing in flatten()) shows a ~2% speedup on SS and a 50% speedup on a simple rope micro-bench I wrote.  So I propose the following optimization:
 - Create a per-compartment ballast.
 - In js_ConcatStrings, bump a counter in the ballast instead of malloc'ing.  If the counter hits a constant max limit, malloc as usual.
 - In flatten() first try to malloc the buffer, decrementing the ballast counter on success, consuming the ballast on failure.  On OOM, set a bit in the compartment so that subsequent js_ConcatStrings OOM.

Alternatively, we could just make JSString::chars() (and its 500 callers) fallible...

Comment 1

[Luke Wagner [:luke]]

Attachment: WIP 1

Comment 2

[Luke Wagner [:luke]]

Attachment: WIP 2

Comment 3

[Luke Wagner [:luke]]

Attachment: perf results

WIP 2 gives a 1.4% and 1.6% speedup on SS/V8, respectively.

I started with a small 16K ballast, but, as I should have guessed, when running anything that creates garbage, this gets exhausted immediately.  So I pumped the buffer up to 32M.  Before the kind reader explodes, consider:
 1. in js_ConcatStrings, ballast memory is used instead of malloc'd memory, so, initially, there is not change in aggregate usage. 
 2. now, on JSString::flatten, the string malloc's its own buffer (it does not consume the ballast, lest the ballast turn into a general heap), so that means we are potentially consuming 2x more memory for string buffers, but:
  a. ballast memory is only touched on oom, so its not in the working set
  b. every gc shrinks the ballast to its minimum size, so it's a temporal waste.  Assuming that we GC when the browser is idle, and since people using Task Manager to "benchmark" our memory usage tend to (in posts I've seen) measure when the browser is idle, this shouldn't be a problem.

Comment 4

[Luke Wagner [:luke]]

Oh, and, to be clear:
 3. as implied by 2b, this 32M is allocated lazily (doubling)

Comment 5

[Nicholas Nethercote [inactive]]

This should be resolved WONTFIX now that 609440 is going ahead, right?

Comment 6

[Luke Wagner [:luke]]

Yes.