Skip to content

perf(formatter): Allocation free interned#7443

Closed
MichaReiser wants to merge 1 commit intobest-fitting-reduce-allocationsfrom
allocation-free-interned
Closed

perf(formatter): Allocation free interned#7443
MichaReiser wants to merge 1 commit intobest-fitting-reduce-allocationsfrom
allocation-free-interned

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Sep 16, 2023

Summary

This PR removes the need for an Rc to implement interned by using the Buffer as an interner

  • Create an ID for the interned content
  • Wrap the content in StartInterned and EndInterned tag with the corresponding ID and write the content directly to the target Buffer.
  • Write a Reference(id) node to reference to the interned content.

The Printer skips interned content but writes it to an internal Index. The Printer uses the internal Index to resolve the Interned content when it sees a Reference. The fact that ids tend to be monotonic allows the use of a Vec as an Index similar to Group ids

The downside of this approach is that it is more complicated to reason about and it is no longer possible to inspect the content of Reference without tracking all interned content. Another downside is that the IR is slightly misleading because you see Interned content and would assume it gets printed in that place, which is not true. For example, the interned content appears before the best_fitting, but the content only gets printed inside the best-fitting variants. Moving the Interned content into the first best fitting variant doesn't work because that content only gets printed conditionally, meaning the Printer would then not always see the Interned content, failing to resolve the Reference.

image

Alternatives

One alternative that I explored is to use bumpallo instead and use it to allocate BestFitting vectors, dynamic strings, and Rcs. This could improve performance even more because it eliminates the 3-4% drop-time too. However, it comes at the downside that Buffer requires a new 'elements lifetime that needs to be added everywhere (Format, Formatter, Buffer, FormatNode, FormatNodeRule....). I gave up at some point because it made the API significantly more complicated.

Test Plan

cargo test

@MichaReiser
Copy link
Member Author

MichaReiser commented Sep 16, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@MichaReiser MichaReiser force-pushed the allocation-free-interned branch from e462ac3 to 673eeda Compare September 16, 2023 17:22
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 16, 2023

CodSpeed Performance Report

Merging #7443 will degrade performances by 2.45%

Comparing allocation-free-interned (e112b02) with best-fitting-reduce-allocations (2843fc0)

Summary

⚡ 2 improvements
❌ 1 regressions
✅ 22 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark best-fitting-reduce-allocations allocation-free-interned Change
formatter[unicode/pypinyin.py] 3.7 ms 3.6 ms +2.26%
formatter[large/dataset.py] 57.3 ms 56.2 ms +2.01%
linter/all-rules[numpy/ctypeslib.py] 34.2 ms 35 ms -2.45%

@MichaReiser MichaReiser force-pushed the allocation-free-interned branch 2 times, most recently from 07d3611 to 4ae041f Compare September 16, 2023 18:21
@MichaReiser MichaReiser added performance Potential performance improvement formatter Related to the formatter labels Sep 16, 2023
@MichaReiser MichaReiser force-pushed the allocation-free-interned branch 2 times, most recently from 1b77a35 to 958c8a0 Compare September 16, 2023 18:53
@MichaReiser MichaReiser changed the base branch from main to best-fitting-reduce-allocations September 16, 2023 19:06
@MichaReiser MichaReiser force-pushed the allocation-free-interned branch from 958c8a0 to a2f8e71 Compare September 16, 2023 19:06
@MichaReiser MichaReiser force-pushed the allocation-free-interned branch 3 times, most recently from 3283f0f to a638734 Compare September 16, 2023 19:24
@MichaReiser MichaReiser force-pushed the best-fitting-reduce-allocations branch from ddd5e34 to 2843fc0 Compare September 17, 2023 13:58
@MichaReiser MichaReiser force-pushed the allocation-free-interned branch from a638734 to e112b02 Compare September 17, 2023 13:59
@MichaReiser
Copy link
Member Author

We may want to bring this back when working on other best fitting layouts but closing for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

formatter Related to the formatter performance Potential performance improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant