Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[opentype/gpos] use plain struct instead of pointer #117

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Conversation

benoitkugler
Copy link
Contributor

During some benchmarks, I found tables.PairSet.FindGlyph was a really high source of heap allocations.
Using a plain struct (probably) allows the compiler/runtime to allocate it on the stack, resulting in a sizable reduction of heap allocations.

@whereswaldon
Copy link
Member

@benoitkugler Do you have before/after benchmark info you could share?

@benoitkugler
Copy link
Contributor Author

@benoitkugler Do you have before/after benchmark info you could share?

Here you are : the code simply runs Shaper.Shape, with a font using a GPOS table (Arimo).

Code

face := loadOpentypeFont(b, "/usr/share/fonts/truetype/croscore/Arimo-Regular.ttf")
textInput := []rune("Une jolie phrase en français !")
input := Input{
	Text:      textInput,
	RunStart:  0,
	RunEnd:    len(textInput),
	Direction: di.DirectionLTR,
	Size:      16 * 72,
	Face:      face,
	Script:    language.Latin,
	Language:  language.NewLanguage("fr"),
}
shaper := HarfbuzzShaper{}

b.ResetTimer()

for i := 0; i < b.N; i++ {
	_ = shaper.Shape(input)
}

Benchmark stats

       │   old.txt   │              new.txt               │
       │   sec/op    │   sec/op     vs base               │
GPOS-4   40.91µ ± 2%   37.47µ ± 5%  -8.41% (p=0.000 n=10)

       │   old.txt    │               new.txt                │
       │     B/op     │     B/op      vs base                │
GPOS-4   15.94Ki ± 0%   11.88Ki ± 0%  -25.48% (p=0.000 n=10)

       │  old.txt   │              new.txt               │
       │ allocs/op  │ allocs/op   vs base                │
GPOS-4   253.0 ± 0%   227.0 ± 0%  -10.28% (p=0.000 n=10)

Memory profile - Before:

old

Memory profile - After :

new

@andydotxyz
Copy link
Contributor

Has the "After" trimmed the RunN contribution or did that go to 0?
I can't do the maths accurately on the fly but it looks like memory is just being moved to other places rather than reduced overall.

@benoitkugler
Copy link
Contributor Author

Has the "After" trimmed the RunN contribution or did that go to 0? I can't do the maths accurately on the fly but it looks like memory is just being moved to other places rather than reduced overall.

My screenshot was incomplete, see the new one :

Capture d’écran du 2023-11-29 12-15-00

@whereswaldon
Copy link
Member

I'm able to replicate the benchmark improvements:

$ benchstat before.out after.out
goos: linux
goarch: amd64
pkg: github.com/go-text/typesetting/shaping
cpu: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
       │ before.out  │             after.out              │
       │   sec/op    │   sec/op     vs base               │
GPOS-8   24.60µ ± 1%   22.76µ ± 1%  -7.48% (p=0.000 n=10)

       │  before.out   │              after.out               │
       │     B/op      │     B/op      vs base                │
GPOS-8   14.043Ki ± 0%   9.980Ki ± 0%  -28.93% (p=0.000 n=10)

       │ before.out │             after.out              │
       │ allocs/op  │ allocs/op   vs base                │
GPOS-8   219.0 ± 0%   193.0 ± 0%  -11.87% (p=0.000 n=10)

Even if this is just shifting an allocation to the caller's stack, the result is that memory is getting re-allocated fewer times during the shaping process, so it's still a net win.

Copy link
Contributor

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification :)

@andydotxyz andydotxyz merged commit 78ebbf6 into main Dec 6, 2023
20 checks passed
@andydotxyz andydotxyz deleted the gpos-alloc branch December 6, 2023 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants