Bug 185967 - Rope string sweeping should avoid touching unresolved ropes
Summary: Rope string sweeping should avoid touching unresolved ropes
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-24 16:45 PDT by Filip Pizlo
Modified: 2018-05-30 08:29 PDT (History)
7 users (show)

See Also:


Attachments
work in progress (40.75 KB, patch)
2018-05-24 16:47 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
maybe it'll work (53.65 KB, patch)
2018-05-25 15:27 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
seems to kinda work (56.45 KB, patch)
2018-05-25 17:09 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (57.49 KB, patch)
2018-05-26 17:04 PDT, Filip Pizlo
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-05-24 16:45:27 PDT
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.
Comment 1 Filip Pizlo 2018-05-24 16:47:21 PDT
Created attachment 341234 [details]
work in progress
Comment 2 Filip Pizlo 2018-05-25 15:27:45 PDT
Created attachment 341338 [details]
maybe it'll work
Comment 3 Filip Pizlo 2018-05-25 16:41:57 PDT
This patch has a crash bug, it's not any faster, and it wastes a lot of memory.

I'm investigating. :-/
Comment 4 Filip Pizlo 2018-05-25 17:09:45 PDT
Created attachment 341361 [details]
seems to kinda work

But it's not a speed-up.
Comment 5 Filip Pizlo 2018-05-25 17:10:39 PDT
(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.
Comment 6 Filip Pizlo 2018-05-26 16:57:53 PDT
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.
Comment 7 Filip Pizlo 2018-05-26 17:04:37 PDT
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.
Comment 8 EWS Watchlist 2018-05-26 17:06:36 PDT
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.
Comment 9 Yusuke Suzuki 2018-05-30 08:29:29 PDT
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.