Flattening refactor

[kvark]

Warning: this would be difficult to review because of large amounts of moved code. Sorry!

Changes:
~~ - (nit) renamed FlattenContext -> Roller to give it a little more flavor, given the FrameContext defined in the same file.~~

• detached flattening from resource cache by wrapping the font instance map in an Arc and passing it to the FlattenContext directly
• (major) moved all the flattening logic from FrameContext into FlattenContext (oh, names...), which appears to have more sense to me architecturally, and allows to reduce the number of parameters we pass to flatten functions (see line count difference)

This is one more step towards moving the scene building in a separate thread. Biggest things left are:

• clip-scroll tree
• scene

cc @glennw @nical

---

This change is 

[glennw]

Do we need a gecko try for this, or does it involve no functional changes?

[kvark]

No functional change is expected here. I believe local tests are sufficient.

[glennw]

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[glennw]

@kvark This looks fine to me.

It will be interesting to see if reading the RWLock on each text run ever shows up in a profile (I don't think it will).

I'm not super keen on the Roller name - it doesn't seem very descriptive to me. But naming is hard - we can always change the name later on if we decide to, so that doesn't need to block this landing. Could we add a description in the source code of what it means though?

I'll leave it to you if you want to do a gecko try run, this seems fairly straightforward though, so r=me.

Thanks!

[glennw]

@kvark I guess having FontInstanceMap as a RWLock can introduce race conditions if someone updates the font instances while a DL is processing (e.g. some text runs may end up not finding the font instance, and other ones in the same DL will).

I don't think it's a big deal - since it can only happen if you're using the API incorrectly.

If we did ever run into perf issues reading the RW lock, or wanted to avoid races, we could probably instead clone the FontInstanceMap once at the start of each job.

[kvark]

@glennw thanks for feedback!

I'm not super keen on the Roller name

I'll rename it back.

It will be interesting to see if reading the RWLock on each text run ever shows up in a profile (I don't think it will).

Yes. Not sure how else we can provide this font instance data. RwLock was sort of a last resort for me.

I guess having FontInstanceMap as a RWLock can introduce race conditions if someone updates the font instances while a DL is processing (e.g. some text runs may end up not finding the font instance, and other ones in the same DL will).

My understanding was that it's going to be RenderBackend responsibility to ensure that font instance updates and removals (but not additions) are deferred until all pending scene builds (that rely on the older font instances) are done.

[glennw]

@bors-servo r+ 🚀

[bors-servo]

📌 Commit 7269276 has been approved by glennw

[nical]

My understanding was that it's going to be RenderBackend responsibility to ensure that font instance updates and removals (but not additions) are deferred until all pending scene builds (that rely on the older font instances) are done.

Yeah we need to pipeline resource updates with scene building (any resource update coming after a scene must wait for that scene to be built) otherwise frame consistency would break. It should be fairly easy to do.

[bors-servo]

⌛ Testing commit 7269276 with merge 844779d...

[bors-servo]

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing 844779d to master...

[staktrace]

@kvark The latest WR update try push is showing a bunch of crashes, this PR is the most likely culprit in the regression range. I'm doing a try push to confirm but you might want a head start on seeing if there's any obvious problem. Details at bug 1410893 comment 9 onwards.

[kvark]

@staktrace thanks for the heads up! this is unexpected. Looking now...