Bug 185967 – Rope string sweeping should avoid touching unresolved ropes

• Component: JavaScriptCore
• Status: NEW

[Filip Pizlo]

Especially when ropes are being used in anger, a lot of JSRopeString objects are never resolved.  Thus they don't need destruction.  Since destruction is expensive, that would be useful to know.

Ropes are usually unresolved because people do something like this:

var result = "";
for (things)
    result += stuff
use result

In that case, we will create a JSRopeString for each iteration of the loop, but we will only resolve the last one.  This comes up over and over again.  For example, this:

"foo" + bar + "baz" + fuzz + "buzz" + thingy + "stuff"

will create three JSRopeStrings, but only the last one will get resolved.

[Filip Pizlo]

Created attachment 341234 [details]
work in progress

[Filip Pizlo]

Created attachment 341338 [details]
maybe it'll work

[Filip Pizlo]

This patch has a crash bug, it's not any faster, and it wastes a lot of memory.

I'm investigating. :-/

[Filip Pizlo]

Created attachment 341361 [details]
seems to kinda work

But it's not a speed-up.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #4)
> Created attachment 341361 [details]
> seems to kinda work
> 
> But it's not a speed-up.

Or, rather, it's a *very small* speed-up.  I'll play with it a bit more before giving up.

[Filip Pizlo]

Seems to not be super great for perf.  If anything, JetStream might be slower.

I think that part of the problem is that those blocks that have unresolved rope strings tend to also not be completely empty, so we have to walk over them anyway.

The high cost of creating ropes might also be so great as to dominate over the benefit of reducing sweep cost.

[Filip Pizlo]

Created attachment 341413 [details]
the patch

Tiny splay speed-up.  The speed-up is minuscule - around 1% - but significant.  I confirmed it in run-jsc-benchmarks, in-browser JetStream, and running the benchmark manually.  It could be an even bigger speed-up if I made bump'n'pop's pop path faster.

[EWS Watchlist]

Attachment 341413 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSRopeStringHeapCellType.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 341413 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341413&action=review

r=me

> Source/JavaScriptCore/heap/MarkedBlock.cpp:460
> +void MarkedBlock::Handle::sweep(FreeList* freeList)

Replacing the position of `MarkedBlock::Handle::sweep` and `MarkedBlock::Handle::finishSweepWithNoDestructors` would make diff clean.