1705777 - Refactor the rope flattening implementation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jon Coppeard (:jonco)]

The rope flattening code has become big and gnarly. I was originally looking into changing the implementation, but ended up doing a series of refactorings while coming to understand it.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1705777 - Factor out a function to set the JSString::LATIN1_CHARS_BIT flag based on the character type r?jandem

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1705777 - Factor out rope pre-barrier into a separate method and tidy up r?jandem

Depends on D112386

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1705777 - Handle allocation failure in one place and remove maybecx parameter from flattening methods r?jandem

Depends on D112387

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1705777 - Make the main flattenInternal method static r?jandem

I found it confusing using |this| in a method that operated on a many different
nodes. This makes it static and passes the root as a named parameter so it's
clearer what's going on.

Depends on D112388

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1705777 - Factor out a function to update the list of buffers associated with nursery cells when we transfer a buffer between cells r?jandem

Depends on D112389

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1705777 - Move simulated traversal part of algorithm into the main loop r?jandem

Currently when resuing the leftmost leaf's buffer we have duplicate code to run
the traversal up until we reach the leftmost rope, because the left child has
been already been processed as a sepcial case. This removes this and checks a
flag in the main traversal code to handle this situation.

Depends on D112390

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1705777 - Factor out a function to check whether we can reuse the buffer of the leftmost leaf node r?jandem

Depends on D112391

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1705777 - Rearrange finish_node so the function exit is at the end of the function r?jandem

Depends on D112392

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Bug 1705777 - Replace bufferIfNursery with tenured checks r?jandem

This ends up doing the same thing but is easer to follow IMO.

Depends on D112393

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db751afb6234
Factor out a function to set the JSString::LATIN1_CHARS_BIT flag based on the character type r=jandem
https://hg.mozilla.org/integration/autoland/rev/420b8e7515ec
Factor out rope pre-barrier into a separate method and tidy up r=jandem
https://hg.mozilla.org/integration/autoland/rev/a4ccc315540c
Handle allocation failure in one place and remove maybecx parameter from flattening methods r=jandem
https://hg.mozilla.org/integration/autoland/rev/8c1078f3f43c
Make the main flattenInternal method static r=jandem
https://hg.mozilla.org/integration/autoland/rev/b4118eb87e2a
Factor out a function to update the list of buffers associated with nursery cells when we transfer a buffer between cells r=jandem
https://hg.mozilla.org/integration/autoland/rev/a0bca47b08bf
Move simulated traversal part of algorithm into the main loop r=jandem
https://hg.mozilla.org/integration/autoland/rev/7bd9513c661a
Factor out a function to check whether we can reuse the buffer of the leftmost leaf node r=jandem
https://hg.mozilla.org/integration/autoland/rev/0f2f2be13e03
Rearrange finish_node so the function exit is at the end of the function r=jandem
https://hg.mozilla.org/integration/autoland/rev/0b87e36e899f
Replace bufferIfNursery with tenured checks r=jandem

Comment 11

[Sandor Molnar[:smolnar]]

Backed out for causing for causing multiple mochitest failures

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

Push with failures

Failure log #1
Failure log #2
Failure log #3
Failure log #4

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29e0af6b9f84
Factor out a function to set the JSString::LATIN1_CHARS_BIT flag based on the character type r=jandem
https://hg.mozilla.org/integration/autoland/rev/7dc0ab701d45
Factor out rope pre-barrier into a separate method and tidy up r=jandem
https://hg.mozilla.org/integration/autoland/rev/b8507d2f588b
Handle allocation failure in one place and remove maybecx parameter from flattening methods r=jandem
https://hg.mozilla.org/integration/autoland/rev/00c304e03e82
Make the main flattenInternal method static r=jandem
https://hg.mozilla.org/integration/autoland/rev/4aa2f490e775
Factor out a function to update the list of buffers associated with nursery cells when we transfer a buffer between cells r=jandem
https://hg.mozilla.org/integration/autoland/rev/e668182d6593
Move simulated traversal part of algorithm into the main loop r=jandem
https://hg.mozilla.org/integration/autoland/rev/e3d9608fcda6
Factor out a function to check whether we can reuse the buffer of the leftmost leaf node r=jandem
https://hg.mozilla.org/integration/autoland/rev/7a5cf9e0810a
Rearrange finish_node so the function exit is at the end of the function r=jandem
https://hg.mozilla.org/integration/autoland/rev/51b03bdd3bb4
Replace bufferIfNursery with tenured checks r=jandem

Comment 13

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/29e0af6b9f84
https://hg.mozilla.org/mozilla-central/rev/7dc0ab701d45
https://hg.mozilla.org/mozilla-central/rev/b8507d2f588b
https://hg.mozilla.org/mozilla-central/rev/00c304e03e82
https://hg.mozilla.org/mozilla-central/rev/4aa2f490e775
https://hg.mozilla.org/mozilla-central/rev/e668182d6593
https://hg.mozilla.org/mozilla-central/rev/e3d9608fcda6
https://hg.mozilla.org/mozilla-central/rev/7a5cf9e0810a
https://hg.mozilla.org/mozilla-central/rev/51b03bdd3bb4

Comment 14

[B.J. Herbison]

Mozregression suggests that this fix may have prevented some menus from appearing on the eBay Purchase History page. See bug 1707423.