perf(rome_formatter): Add Buffer.elements
#3156
Conversation
✅ Deploy Preview for rometools canceled.
|
|
!bench_formatter |
MichaReiser
changed the title
PreambleBufferBuffer.elements
MichaReiser
force-pushed
the
perf/reduce-preamble-use
branch
from
fd6e822 to
2b24bec
Compare
| @@ -34,8 +34,7 @@ const m2 = module/* B1 */{ | |||
| export { foo }; /*A5*/ | |||
| /*A6*/ | |||
| -}; /*A7*/ /*A8*/ | |||
| +} /*A7*/ | |||
| +/*A8*/ | |||
| +} /*A7*/ /*A8*/ | |||
There was a problem hiding this comment.
I'll have a look at the comments as part of the comments refactoring (which is why this PR shouldn't be part of the 0.9 release)
MichaReiser
force-pushed
the
perf/reduce-preamble-use
branch
from
2b24bec to
0613914
Compare
|
!bench_formatter |
Formatter Benchmark Results |
|
Comparing perf(rome_formatter): Add
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 |
431ms  from 7ms |
 |
| JS Parse & Compile Motorola Moto G Power, 3G connection |
1.44s  from 27ms |
|
| Total JavaScript Size in Bytes Chrome Desktop |
4.02 MB  from 86.8 KB |
|
| Total JavaScript Size in Bytes iPhone, 4G LTE |
4.02 MB from 86.8 KB |
|
| Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection |
4.02 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, First Contentful Paint on Chrome Desktop, Number of Requests on Motorola Moto G Power, 3G connection, Time to Interactive on Motorola Moto G Power, 3G connection, Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, First Contentful Paint on iPhone, 4G LTE, Time to Interactive on Chrome Desktop, Speed Index on Chrome Desktop, Largest Contentful Paint on iPhone, 4G LTE, Speed Index on iPhone, 4G LTE, Time to Interactive on iPhone, 4G LTE, 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
| match module_item { | ||
| JsAnyModuleItem::JsAnyStatement(JsAnyStatement::JsEmptyStatement(empty)) => { | ||
| join.entry_no_separator(&empty.format()); | ||
| } | ||
| _ => { | ||
| join.entry(module_item.syntax(), &format_or_verbatim(&module_item)); | ||
| } | ||
| } |
There was a problem hiding this comment.
What did we change/fix here?
There was a problem hiding this comment.
We don't want to insert empty lines if it is an empty statement because that will get removed. This was previously automatically handled inside of entry but turns out to be rather expensive. So it's better to explicitly handle it where it is needed.
| inner: BufferSnapshot, | ||
| has_label: bool, | ||
| } | ||
| pub fn stop(self) -> Recorded<'buf> { |
There was a problem hiding this comment.
Why do we return a newtype here?
There was a problem hiding this comment.
No very specific reason. Felt more complete if stop returns a Recorded than a slice directly. The new type allows us to implement additional helpers in the future
Summary
It turns out, that using the
PreambleBufferin hot code paths is rather expensive. Or in general, nestingBuffers can be rather expensive.I discovered this after I implemented a prototype of a Flat IR for our formatter (#2571) where this problem becomes more prominent because
group,indentetc are no longer "breaking" the buffer chain by writing into a newVecBuffer.This PR introduces a new
Buffer.elements()method that gives access to a slice of all written elements (that are part of this buffer). This allows introspection of the written content very cheaply (may require iteration, but most inspections are only interested in the last element) and in a more natural way (writing custom buffers or even usingmemoizedoften feels overly complicated). Another benefit ofelementsover usinginspector a custom buffer is that it guarantees that rewinding the buffer is correctly taken into account, something that most uses ofinspectdidn't.The new
elementsmethod allows removing most uses of thePreambleBufferby doing a simplelencomparison or inspecting the slice content.This PR deletes the unused
WillBreakBufferandHaslLabelBufferbuffers in favour of eitherMemoized.inspector usingbuffer.elements(). We can re-introduce the buffers if it is indeed necessary for some logic to check if any element breaks.The downside of this is that it slightly leaks the
Bufferextraction as it forces theBufferto now always keep hold of all written elements. For example, this prevents us from having a Printer that prints IR elements as they get formatted. I don't think this is a very practical use case with our current Formatter design as it would mean that the printer has to rewind every time the formatter rewinds... meaning that rewinding will result in more work that must the thrown out the window (and overhead for creating the snapshots).I even considered changing
Formatterto always store aVecBufferbut there are still a few use cases where a customBuffercan be useful and we should continue supporting those. Buffers on their own aren't a problem, only if they get insanely stacked. For example, it would probably be better to apply aGroupsElementsBufferonce in a world where we have a FlatIR rather than nesting it over and over again.Test Plan
cargo tetstThe performance win for our nested IR are less impressive. But it improves performance by roughly 20-25% for a flat IR and more improvements are possible once the
GroupsElementsBufferis no longer necessary.