Skip to content

Add caching to IFont2Font#1179

Merged
tonyqus merged 1 commit into
nissl-lab:masterfrom
lahma:improve-autosizecolumn-perf
Sep 6, 2023
Merged

Add caching to IFont2Font#1179
tonyqus merged 1 commit into
nissl-lab:masterfrom
lahma:improve-autosizecolumn-perf

Conversation

@lahma
Copy link
Copy Markdown
Collaborator

@lahma lahma commented Sep 6, 2023

Adding caching to font creation as it can be quite costly. Showing the difference it makes by creating benchmark for AutoSizeColumn.

NPOI.Benchmarks.AutoSizeColumnBenchmark

Diff Method Mean Error Allocated
Old AutoSizeColumn 1.329 s 0.0156 s 673.13 MB
New 144.3 ms (-89%) 1.94 ms 130.58 MB (-81%)

@lahma lahma mentioned this pull request Sep 6, 2023
@Bykiev
Copy link
Copy Markdown
Collaborator

Bykiev commented Sep 6, 2023

The caching result is really impressive!
@lahma, maybe ceiling FontHeightInPoints will be a better approach?

@tonyqus
Copy link
Copy Markdown
Member

tonyqus commented Sep 6, 2023

LGTM

@tonyqus tonyqus added this to the NPOI 2.7.0 milestone Sep 6, 2023
@tonyqus tonyqus merged commit ece703e into nissl-lab:master Sep 6, 2023
@lahma lahma deleted the improve-autosizecolumn-perf branch September 6, 2023 18:19
@lahma
Copy link
Copy Markdown
Collaborator Author

lahma commented Sep 6, 2023

The caching result is really impressive! @lahma, maybe ceiling FontHeightInPoints will be a better approach?

I'm hoping that generally the value is always an integer so shouldn't matter that much. Just wanted to make sure nobody creates a font-loader bomb with an excel that has different font size in every cell and just by decimals.

@Sappharad
Copy link
Copy Markdown
Contributor

Thank you for improving the performance so soon after I commented on it yesterday, the benchmarks you posted look great! I rolled back to 2.5.6 but look forward to upgrading to 2.7.0 when it's out.

@JimBobSquarePants
Copy link
Copy Markdown

This is the correct approach. ClosedXML has introduced incorrect measurements by using custom measurements based upon the advance of individual glyphs.

I've worked hard to ensure that text measurement is fast and accurate (proper bidi layout and shaping is incredibly complicated) but would always recommend a font file is primed for consumption.

@tonyqus
Copy link
Copy Markdown
Member

tonyqus commented Sep 9, 2023

@JimBobSquarePants It's nice to meet Six Labors staff here.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants