go-text / typesetting

High quality text shaping in pure Go.
Other
101 stars 10 forks source link

Support for monospace property #61

Closed benoitkugler closed 1 year ago

benoitkugler commented 1 year ago

This PR add support to check if a font is mono-spaced. See #57 for discussion.

whereswaldon commented 1 year ago

Pushed benchmarks in order to evaluate the impact here:

goos: linux
goarch: amd64
pkg: github.com/go-text/typesetting/shaping
cpu: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
                                     │ without-monospace.txt │          with-monospace.txt           │
                                     │        sec/op         │    sec/op      vs base                │
FontMetadata/arabic:amiri_regular-8             3.146µ ± ∞ ¹   31.650µ ± ∞ ¹  +906.04% (p=0.008 n=5)
FontMetadata/lating:go_regular-8                4.342µ ± ∞ ¹   14.999µ ± ∞ ¹  +245.44% (p=0.008 n=5)
FontMetadata/lating:go_mono-8                   4.380µ ± ∞ ¹   20.503µ ± ∞ ¹  +368.11% (p=0.008 n=5)
FontMetadata/lating:roboto_regular-8            2.964µ ± ∞ ¹   16.621µ ± ∞ ¹  +460.76% (p=0.008 n=5)
geomean                                         3.649µ          20.06µ        +449.58%
¹ need >= 6 samples for confidence interval at level 0.95

                                     │ without-monospace.txt │           with-monospace.txt            │
                                     │         B/op          │      B/op       vs base                 │
FontMetadata/arabic:amiri_regular-8            3.933Ki ± ∞ ¹   69.776Ki ± ∞ ¹  +1674.30% (p=0.008 n=5)
FontMetadata/lating:go_regular-8               9.673Ki ± ∞ ¹   34.696Ki ± ∞ ¹   +258.70% (p=0.008 n=5)
FontMetadata/lating:go_mono-8                  9.720Ki ± ∞ ¹   31.743Ki ± ∞ ¹   +226.58% (p=0.008 n=5)
FontMetadata/lating:roboto_regular-8           2.304Ki ± ∞ ¹   27.515Ki ± ∞ ¹  +1094.36% (p=0.008 n=5)
geomean                                        5.402Ki          38.13Ki         +605.87%
¹ need >= 6 samples for confidence interval at level 0.95

                                     │ without-monospace.txt │          with-monospace.txt          │
                                     │       allocs/op       │  allocs/op    vs base                │
FontMetadata/arabic:amiri_regular-8              33.00 ± ∞ ¹    72.00 ± ∞ ¹  +118.18% (p=0.008 n=5)
FontMetadata/lating:go_regular-8                 32.00 ± ∞ ¹    68.00 ± ∞ ¹  +112.50% (p=0.008 n=5)
FontMetadata/lating:go_mono-8                    32.00 ± ∞ ¹    68.00 ± ∞ ¹  +112.50% (p=0.008 n=5)
FontMetadata/lating:roboto_regular-8             36.00 ± ∞ ¹   117.00 ± ∞ ¹  +225.00% (p=0.008 n=5)
geomean                                          33.21          79.00        +137.88%
¹ need >= 6 samples for confidence interval at level 0.95

The metadata extraction process gets much slower as a result of these changes. That makes sense, as we need to look much deeper within the font. However, it seems that we loop over the entire contents of a monospace font to check if it's monospace?

https://github.com/go-text/typesetting/pull/61/files#diff-f290e40b1882992a7ceb2ee5fc8cfb7c8cf35242a63e770f6a25f10ae9854141R103

I guess I misunderstood this comment to mean that there was font metadata we could consume instead of searching all of the glyphs. Is this really the only option? If so, we just have to take this hit.

benoitkugler commented 1 year ago

I guess I misunderstood this comment to mean that there was font metadata we could consume instead of searching all of the glyphs. Is this really the only option? If so, we just have to take this hit.

Oh you are right, I was not precise enough. Actually the metadata is often broken, that is why fontconfig checks it the slow way. That is indeed an order slower compare to just checking flags..

Maybe we could trust a flag if present ? Hum, let met check this...

benoitkugler commented 1 year ago

The last commits use a faster implementation, only looking at every width when no fixed width flag is set. The performance hit should hopefully be acceptable.

benoitkugler commented 1 year ago

The code looks good but unfortunately this brings back the golang.org/x/sys package from 0.5.0 which requires go 1.17

Woups, sorry.

EDIT : fixed by 71e9b7a

whereswaldon commented 1 year ago

Fixed the conflicts in the benchmarks.

whereswaldon commented 1 year ago

Well, I screwed up the first rebase, but I think it's right now. Sorry. :D

whereswaldon commented 1 year ago

I force-pushed again to remove that same lating typo from the benchmarks and to add the FontMetadataAndParse benchmarks.

I've run before/after benchmarks on this branch:

goos: linux
goarch: amd64
pkg: github.com/go-text/typesetting/shaping
cpu: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
                                            │ without-monospace.txt │           with-monospace.txt           │
                                            │        sec/op         │    sec/op      vs base                 │
FontMetadata/arabic:amiri_regular-8                    3.170µ ± ∞ ¹   70.587µ ± ∞ ¹  +2126.72% (p=0.008 n=5)
FontMetadata/latin:go_regular-8                        4.423µ ± ∞ ¹   18.396µ ± ∞ ¹   +315.92% (p=0.008 n=5)
FontMetadata/latin:go_mono-8                           4.508µ ± ∞ ¹   17.502µ ± ∞ ¹   +288.24% (p=0.008 n=5)
FontMetadata/latin:roboto_regular-8                    3.074µ ± ∞ ¹   17.015µ ± ∞ ¹   +453.51% (p=0.008 n=5)
FontMetadataAndParse/arabic:amiri_regular-8            3.066m ± ∞ ¹    3.138m ± ∞ ¹     +2.37% (p=0.016 n=5)
FontMetadataAndParse/latin:go_regular-8                415.2µ ± ∞ ¹    424.0µ ± ∞ ¹     +2.12% (p=0.016 n=5)
FontMetadataAndParse/latin:go_mono-8                   455.9µ ± ∞ ¹    465.9µ ± ∞ ¹          ~ (p=0.690 n=5)
FontMetadataAndParse/latin:roboto_regular-8            552.5µ ± ∞ ¹    564.3µ ± ∞ ¹     +2.15% (p=0.008 n=5)
geomean                                                53.00µ          138.5µ         +161.28%
¹ need >= 6 samples for confidence interval at level 0.95

                                            │ without-monospace.txt │            with-monospace.txt            │
                                            │         B/op          │      B/op        vs base                 │
FontMetadata/arabic:amiri_regular-8                   3.933Ki ± ∞ ¹   219.074Ki ± ∞ ¹  +5470.70% (p=0.008 n=5)
FontMetadata/latin:go_regular-8                       9.673Ki ± ∞ ¹    41.493Ki ± ∞ ¹   +328.97% (p=0.008 n=5)
FontMetadata/latin:go_mono-8                          9.720Ki ± ∞ ¹    38.540Ki ± ∞ ¹   +296.51% (p=0.008 n=5)
FontMetadata/latin:roboto_regular-8                   2.304Ki ± ∞ ¹    27.546Ki ± ∞ ¹  +1095.72% (p=0.008 n=5)
FontMetadataAndParse/arabic:amiri_regular-8           3.487Mi ± ∞ ¹     3.697Mi ± ∞ ¹     +6.03% (p=0.008 n=5)
FontMetadataAndParse/latin:go_regular-8               375.5Ki ± ∞ ¹     407.3Ki ± ∞ ¹     +8.48% (p=0.008 n=5)
FontMetadataAndParse/latin:go_mono-8                  420.8Ki ± ∞ ¹     449.6Ki ± ∞ ¹     +6.85% (p=0.008 n=5)
FontMetadataAndParse/latin:roboto_regular-8           505.0Ki ± ∞ ¹     530.3Ki ± ∞ ¹     +5.00% (p=0.008 n=5)
geomean                                               62.82Ki           208.3Ki         +231.60%
¹ need >= 6 samples for confidence interval at level 0.95

                                            │ without-monospace.txt │          with-monospace.txt          │
                                            │       allocs/op       │  allocs/op    vs base                │
FontMetadata/arabic:amiri_regular-8                     33.00 ± ∞ ¹    74.00 ± ∞ ¹  +124.24% (p=0.008 n=5)
FontMetadata/latin:go_regular-8                         32.00 ± ∞ ¹    70.00 ± ∞ ¹  +118.75% (p=0.008 n=5)
FontMetadata/latin:go_mono-8                            32.00 ± ∞ ¹    70.00 ± ∞ ¹  +118.75% (p=0.008 n=5)
FontMetadata/latin:roboto_regular-8                     36.00 ± ∞ ¹   117.00 ± ∞ ¹  +225.00% (p=0.008 n=5)
FontMetadataAndParse/arabic:amiri_regular-8            43.77k ± ∞ ¹   43.82k ± ∞ ¹    +0.10% (p=0.008 n=5)
FontMetadataAndParse/latin:go_regular-8                2.776k ± ∞ ¹   2.814k ± ∞ ¹    +1.37% (p=0.008 n=5)
FontMetadataAndParse/latin:go_mono-8                   2.776k ± ∞ ¹   2.814k ± ∞ ¹    +1.37% (p=0.008 n=5)
FontMetadataAndParse/latin:roboto_regular-8            4.488k ± ∞ ¹   4.569k ± ∞ ¹    +1.80% (p=0.008 n=5)
geomean                                                 455.1          713.6         +56.79%
¹ need >= 6 samples for confidence interval at level 0.95

Extracting font metadata is significantly more expensive than it used to be, but we will rarely (if ever) extract metadata from a font that we aren't also going to parse. The FontMetadataAndParse benchmark measures changes to the performance of doing both operations together, which is what the toolkits will likely experience.

Based on that:

Overall, not a huge hit relative to where we were before.

benoitkugler commented 1 year ago

Final change on the deps :

Should be ready now