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

[varStore] Improve optimize algorithm #3124

Merged
merged 8 commits into from May 24, 2023
Merged

[varStore] Improve optimize algorithm #3124

merged 8 commits into from May 24, 2023

Conversation

behdad
Copy link
Member

@behdad behdad commented May 24, 2023

This was always supposed to be how it works, but was missed in the initial implementation apparently.

I should probably add extensive tests for the optimization algorithm...

This was always supposed to be how it works, but was missed in
the initial implementation apparently.
@behdad behdad requested a review from anthrotype May 24, 2023 17:08
@behdad
Copy link
Member Author

behdad commented May 24, 2023

Saves 408 bytes building NotoSansArabic.

@anthrotype
Copy link
Member

hm in the absence of a test that touches those lines, it's a bit too hard for me to reivew so I'll pass. feel free to merge, this is your code

@behdad behdad requested review from madig and belluzj May 24, 2023 17:15
@behdad
Copy link
Member Author

behdad commented May 24, 2023

Adding Jany and Nikolaus in case one of them wants to read and understand the algorithm. I'm willing to add more comments. And yes I'll think about adding tests.

Previously we were disregarding the best_gain. Ouch!
@behdad
Copy link
Member Author

behdad commented May 24, 2023

I documented the algorithm.

@rsheeter
Copy link
Collaborator

The doc comment is most appreciated. +1 to tests.

Saves 408 bytes building NotoSansArabic.

Are there cases where it saves more?

@behdad
Copy link
Member Author

behdad commented May 24, 2023

The doc comment is most appreciated. +1 to tests.

Saves 408 bytes building NotoSansArabic.

Are there cases where it saves more?

In NotoSans it saved 2kb, but that's 0.1%. I don't expect it to be much; in the 1kb to 2kb range, but hey, it's free savings.

Doesn't hit all branches yet.
@behdad
Copy link
Member Author

behdad commented May 24, 2023

In NotoSans it saved 2kb

Another bugfix (thanks to new tests), and it saves 4.5kb now.

Previously we were not accounting for the LOffset to VarData.
@behdad
Copy link
Member Author

behdad commented May 24, 2023

Saves 408 bytes building NotoSansArabic.

Now at 936 bytes after a few other fixes.

@behdad behdad merged commit 22c76c4 into main May 24, 2023
10 checks passed
@behdad behdad deleted the varStore-optimize-fix branch May 24, 2023 19:40
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.

None yet

3 participants