refactor(rome_formatter): Flat IR #3160
Conversation
✅ Deploy Preview for rometools canceled.
|
|
The performance is regressing at the moment because each group is adding another layer of The comments refactoring should remove the need for the |
MichaReiser
force-pushed
the
refactor/flat-ir
branch
from
9879b5b to
6d0c6e7
Compare
MichaReiser
force-pushed
the
refactor/flat-ir
branch
from
6d0c6e7 to
a005266
Compare
MichaReiser
force-pushed
the
refactor/flat-ir
branch
from
a005266 to
07236ed
Compare
MichaReiser
force-pushed
the
refactor/flat-ir
branch
from
07236ed to
2b6fada
Compare
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
MichaReiser
force-pushed
the
refactor/flat-ir
branch
from
9a56209 to
0c9e16a
Compare
MichaReiser
changed the base branch from
main
to
feat/assert-comments-formatting
| @@ -7,8 +7,6 @@ repository = "https://github.com/rome/tools" | |||
| description = "WebAssembly bindings to the Rome Workspace API" | |||
| license = "MIT" | |||
|
|
|||
| [package.metadata.wasm-pack.profile.dev] | |||
There was a problem hiding this comment.
The wasm optimisations were necessary to avoid the playground from stack overflowing. This seems to be no longer necessary and I believe it comes from the fact that the Display implementation for &[FormatElement] (used in the RomeIR tab is no longer recursive.
The problem was that the old Display implementation recursed for every container (and, performance wise, created a group and soft block indent vec for every container...).
MichaReiser
force-pushed
the
feat/assert-comments-formatting
branch
from
c568991 to
05aab2b
Compare
MichaReiser
force-pushed
the
refactor/flat-ir
branch
4 times, most recently
from
655ce46 to
9d19c50
Compare
MichaReiser
force-pushed
the
feat/assert-comments-formatting
branch
from
05aab2b to
868e1c9
Compare
MichaReiser
force-pushed
the
refactor/flat-ir
branch
from
9d19c50 to
bc38dac
Compare
MichaReiser
force-pushed
the
feat/assert-comments-formatting
branch
from
868e1c9 to
a731169
Compare
|
Comparing refactor(rome_formatter): Flat IR Snapshot #9 to median since last deploy of rome.tools.
1 page tested
|
| Chrome Desktop | iPhone, 4G LTE | Motorola Moto G Power, 3G connection |
|---|---|---|
 |
 |
 |
Most significant changes
 |
Value | Budget |
|---|---|---|
| JS Parse & Compile iPhone, 4G LTE |
490ms  from 7ms |
 |
| JS Parse & Compile Motorola Moto G Power, 3G connection |
1.55s  from 29ms |
|
| Total JavaScript Size in Bytes Chrome Desktop |
4.04 MB  from 86.8 KB |
|
| Total JavaScript Size in Bytes iPhone, 4G LTE |
4.04 MB from 86.8 KB |
|
| Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection |
4.04 MB from 86.8 KB |
|
28 other significant changes: JS Parse & Compile on Chrome Desktop, First Contentful Paint on Motorola Moto G Power, 3G connection, Largest Contentful Paint on Motorola Moto G Power, 3G connection, Speed Index on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection, Time to Interactive on Motorola Moto G Power, 3G connection, Number of Requests on Motorola Moto G Power, 3G connection, Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, First Contentful Paint on Chrome Desktop, First Contentful Paint on iPhone, 4G LTE, Time to Interactive on Chrome Desktop, Speed Index on iPhone, 4G LTE, Largest Contentful Paint on iPhone, 4G LTE, Time to Interactive on iPhone, 4G LTE, Speed Index on Chrome Desktop, Total Blocking Time on Motorola Moto G Power, 3G connection, Largest Contentful Paint on Chrome Desktop, Total Image Size in Bytes on Chrome Desktop, Total Image Size in Bytes on iPhone, 4G LTE, Total Image Size in Bytes on Motorola Moto G Power, 3G connection, Total HTML Size in Bytes on Chrome Desktop, Total HTML Size in Bytes on iPhone, 4G LTE, Total HTML Size in Bytes on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Chrome Desktop
Calibre: Site dashboard | View this PR | Edit settings | View documentation
|
!bench_formatter |
Formatter Benchmark Results |
|
We use the concept of "signal" inside the analyzer, which has its own semantics and it plays will that infrastructure and the actual tool, the transformer. Here, it seems that the concept has a different semantic. My main concern is the overlap of two concepts that have different meanings but same name. It causes confusion to me. A suggestion, maybe could use "Event" instead - just a suggestion and it's arbitrary -, we have an example of an external crate where it's used: https://github.com/rome/tools/blob/main/xtask/lintdoc/src/main.rs#L221-L243 They look similar to me. |
I'm conflicted on The one downside I see is that What do you think of the name |
crates/rome_formatter/src/lib.rs
Outdated
| @@ -458,6 +579,7 @@ impl std::fmt::Display for FormatError { | |||
| fmt, | |||
| "formatting range {input:?} is larger than syntax tree {tree:?}" | |||
| ), | |||
| FormatError::InvalidDocument(error) => std::write!(fmt, "Invalid document: {error}\n\n This is an internal Rome error. Please open an issue https://github.com/rome/tools/issues."), | |||
There was a problem hiding this comment.
| FormatError::InvalidDocument(error) => std::write!(fmt, "Invalid document: {error}\n\n This is an internal Rome error. Please open an issue https://github.com/rome/tools/issues."), | |
| FormatError::InvalidDocument(error) => std::write!(fmt, "Invalid document: {error}\n\n This is an internal Rome error. Please report it if necessary."), |
The phrase is inline with the other diagnostics. It make sense to show the link, but it should be printed using the Hyperlink markup which handles links inside the terminal, making it clickable.
Maybe @leops has a suggestion on how to do that (maybe doing it automatically for Fatal errors?)
Yeah that works for me |
MichaReiser
force-pushed
the
refactor/flat-ir
branch
from
74ccd47 to
47fdd56
Compare
MichaReiser
force-pushed
the
feat/assert-comments-formatting
branch
from
9f566e7 to
88a9d4f
Compare
MichaReiser
force-pushed
the
refactor/flat-ir
branch
from
47fdd56 to
d54fda4
Compare
MichaReiser
force-pushed
the
refactor/flat-ir
branch
from
d54fda4 to
ca53a38
Compare
Summary
This PR changes our hierarchical Formatter IR to a flat IR as described in #2571.
The formatter IR today is a deeply nested structure where every container (
group,indent, ...) wrap some inner content. The downside of this is that the formatter must allocate a newVecfor every single container which is expensive.This PR changes the formatter IR and replaces (almost all) containers with
Signals. A signal signals adds some formatting to all content up to its corresponding end signal.The upside of using a flat structure is that it is no longer necessary to allocate a new
Vecfor every container, which drastically reduces the number of allocations in the formatter.The main downside is that it is now possible to construct invalid documents if the
startandendsignals don't match. This concern is mitigated by the fact that very few places in the formatter construct the IR elements manually usingFormatElement. The vast majority uses the builders provided byrome_formatterthat are well tested to only create valid documents.Nevertheless, the fact that invalid documents are a possibility requires to change the
Printer::printmethod to return aPrintResultso that it can return anErrif an invalid document is provided.The changes are mostly local to the
rome_formattercrate, proving that we have good abstractions in place.Format Element
This PR removes all
FormatElements with aBox<FormatElement>and replaces them with a newFormatElement::Signalelement. ASignalalways comes in aStartandEndpair where theStartmay store additional information, e.g. the group id.The two containers that haven't been moved out are:
FormatElement::Interned: It's sole purpose is to store aBox<FormatElement>to get cheap writes (and clones).FormatElement::BestFitting: It would be possible to modelBestFittingwith aStart/Endsignal but it significantly increased complexity when measuring if a variant fits because it becomes necessary to skip over later coming variants. That's why I left it for now. UsingBestFittingcomes at a high cost anyway because it requires interning the inner content, and nested best fittings have exponential complexity when printing.Builders
It has been necessary to change the container (
group,indent, ..) builders to now write start/end signals instead of creating a newVecBuffer, writing into that buffer, and then creating an element wrapping the content.Printer
It has been necessary to replace the internal queue of the printer where it stored each element to print together with its
PrintElementArgswith two data structures:CallStack: Stack storing thePrintElementArgs. A new entry is added when printing a start signal and the entry gets popped with the end signal.PrintQueue: A queue tracking which element must be printed next. The queue uses an internal stack of slices to process together with an index pointing to the next element. This avoids copying all elements into the internal queue which would be especially expensive during measuring if the content fits because it would be necessary to copy a whole group or entry into the queue.Performance
Linux
Windows
Mac
Allocations
The number of allocations are drastically reduced but the total allocated bytes increases. My understanding of why the total memory increases is because
Vecs grow relative to their size, meaning that largerVecs may allocate a lot of space for unused "elements".The memory consumption should be of little concerns as these get released as soon as formatting is done.
jquery
At t-gmax:
tex-chtml-full
Test Plan
I added a few new tests for the newly introduced data structures but otherwise mainly relied on the existing tests.