1706694 - Change rope flattening to not overwrite cell header data

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jon Coppeard (:jonco)]

Currently rope flattening traverses the DAG of ropes without using extra storage by storing the parent pointer of an internal node (plus flags) over the cell header field. This is complicated and surprising, and it is also a problem for incremental marking because off-thread marking can't trust the string flag bits to be correct.

I investigated using another approach which does use external storage but this caused a performance regression on the octane PdfJS benchmark which flattens rope DAGs that are thousands of nodes in size.

Instead it should be possible to use a different field to hold the temporary parent pointer, and doing so would be much less troublesome.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1706694 - When flattening ropes, store the parent pointer in the left child field rather than overwriting the cell header r?jandem

This moves the part that sets the charater data pointer to finish_node as this
also shares this field. We pass the parent pointer (and flags) into
first_visit_node and set these after we've extracted the left child pointer.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1706694 - Tidy string flattening to use JSRope* in a few more places r?jandem

Depends on D113316

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8269c01489c9
When flattening ropes, store the parent pointer in the left child field rather than overwriting the cell header r=jandem

Comment 4

[Sandor Molnar[:smolnar]]

Backed out for causing failures in PreWriteBarrierDuringFlattening

Backout link: https://hg.mozilla.org/integration/autoland/rev/e3136f372f53893814058f92df0c80311cba2460

Push with failures

Failure log

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1706694 - Don't overwrite leftmost rope's internals at the start when reusing the leftmost child buffer in string flattening r?jandem

This is unnecessary and was causing problems. We can leave the leftmost rope to
be handled in the same way as all the other ropes, and move the special case to
just avoid copying the chars when reusing the leftmost buffer.

The problem was that now we always barrier ropes, before overwriting the left
child pointer with the parent pointer. If this happens during incremental GC we
must ensure we haven't already overwritten this pointer we the buffer pointer.

This patch also moves changing the leftmost string into a dependent string
until the end.

Depends on D113316

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1706694 - Simplify use of Add/RemoveCellMemory r?jandem

These already check whether the associated cell is tenured so we can remove the extra checks.

Depends on D113317

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a47a2921119c
When flattening ropes, store the parent pointer in the left child field rather than overwriting the cell header r=jandem
https://hg.mozilla.org/integration/autoland/rev/1d6f87631df7
Don't overwrite leftmost rope's internals at the start when reusing the leftmost child buffer in string flattening r=jandem
https://hg.mozilla.org/integration/autoland/rev/abfa26f32f96
Tidy string flattening to use JSRope* in a few more places r=jandem
https://hg.mozilla.org/integration/autoland/rev/d9c03de42812
Simplify use of Add/RemoveCellMemory r=jandem

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/a47a2921119c
https://hg.mozilla.org/mozilla-central/rev/1d6f87631df7
https://hg.mozilla.org/mozilla-central/rev/abfa26f32f96
https://hg.mozilla.org/mozilla-central/rev/d9c03de42812