Bug 105102 - Merge some optimisation works by Michal Srb?
Summary: Merge some optimisation works by Michal Srb?
Status: RESOLVED MOVED
Alias: None
Product: fontconfig
Classification: Unclassified
Component: library (show other bugs)
Version: 2.12
Hardware: Other All
: medium normal
Assignee: fontconfig-bugs
QA Contact: Behdad Esfahbod
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-14 21:46 UTC by Mingcong Bai
Modified: 2018-08-20 21:55 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Mingcong Bai 2018-02-14 21:46:40 UTC
First described in his paper ¹ to optimise various fontconfig algorithms, his work ² has proven to be beneficial especially with LibreOffice. With current 2.12.6 it is practically impossible to scroll through the font listing when a full set of Noto and Noto CJK fonts are installed, scrolling can cause the UI to lock up for more than a second which practically destroys the user experience.

With patches generated from his work - applied upon 2.12.6, this issue has gone away. I was trying to test https://bugs.freedesktop.org/show_bug.cgi?id=105085 with 2.12.92, however, these patches would no longer apply to the newest code, as extensive API changes were made - it would be a shame to waste all these optimisation works...

--------

¹ https://www.theseus.fi/bitstream/handle/10024/124694/Srb_Michal.pdf?sequence=1
² https://github.com/michalsrb/fontconfig
Comment 1 Mingye Wang (Arthur2e5) 2018-02-14 23:11:46 UTC
Attempting some naive rebaseing right now. f93fa33 looks a bit complicated with all the extra operands...
Comment 2 Behdad Esfahbod 2018-02-14 23:16:21 UTC
His work is a great example of how NOT contribute to an Open Source project.  What a shame...
Comment 3 Behdad Esfahbod 2018-02-14 23:28:01 UTC
Some are those are good to grab. Some are good ideas, but should be implemented in a less intrusive manner. Others are just premature optimization without any measurable impact IMO.
Comment 4 Mingye Wang (Arthur2e5) 2018-02-14 23:59:13 UTC
Done-ish at https://github.com/AOSC-Dev/fontconfig/tree/master-reb. Bai can you tell me whether it builds?
Comment 5 Mingcong Bai 2018-02-15 01:27:01 UTC
(In reply to Mingye Wang (Arthur2e5) from comment #4)
> Done-ish at https://github.com/AOSC-Dev/fontconfig/tree/master-reb. Bai can
> you tell me whether it builds?

It does build but we (for transparency's sake I'm currently working with Wang on this) are running into some weird segfaults.
Comment 6 Mingcong Bai 2018-02-15 01:28:17 UTC
(In reply to Behdad Esfahbod from comment #3)
> Some are those are good to grab. Some are good ideas, but should be
> implemented in a less intrusive manner. Others are just premature
> optimization without any measurable impact IMO.

Right, in our interest we will continue to work on merging it with current master. However, if you would like to help/pick out changes you find useful, please do so by all means - after all it is not wise for us to keep a fork.
Comment 7 Behdad Esfahbod 2018-02-15 02:37:37 UTC
Here's my take on the optimizations. First a note: KDE's use of FcFontMatch() with FC_FILE set is bogus and broken. Ignore that.

* 3.4.1 Rewriting FcFontMatch algorithm
-> Valid idea. 10% is worth taking.

* 3.4.2 Value preprocessing
-> Valid concern. It's been on my radar for years. The solution is too invasive for my taste. Another approach might be to add new pattern elements FC_FAMILY_HASH, etc, that retain the (corresponding type of) hash of the string. It will be equally intrusive I'm afraid. But yeah, definitely something to pursue and see if better implementation is possible. Definitely biggest waste of time in current FontConfig.  Side note: if we go all the way to keep hashes of family names, we might end up wanting to use a hashtable for FcFontMatch...

* 3.4.3 Reducing heap allocations
-> Not clear that is measurable. But sure, is not intrusive. I'm open to.

* 3.4.4 Refactoring FcStrCaseWalkerNext
-> Sounds fine. We might not need to triplicate it, maybe just inline that function.

* 3.4.5 Micro-optimization in FcCompareValueList
-> I have a hard time believing the 38% speedup claimed. If it has measurable impact, sounds good to me.

* 3.4.6 Reduce FcObjectFromName call amount
-> Not sure it's worth it; has no measurable impact.

* 3.4.7 Hint branch predictor
-> Good to take, since it's not intrusive. We do these in other libraries.

Anyway, I wish he had done his work in collaboration with us so we could have discussed and integrated this upstream without pain.  Thanks for bringing up.
Comment 8 Mingcong Bai 2018-02-15 02:41:34 UTC
(In reply to Behdad Esfahbod from comment #7)
> Here's my take on the optimizations. First a note: KDE's use of
> FcFontMatch() with FC_FILE set is bogus and broken. Ignore that.
> 
> * 3.4.1 Rewriting FcFontMatch algorithm
> -> Valid idea. 10% is worth taking.
> 
> * 3.4.2 Value preprocessing
> -> Valid concern. It's been on my radar for years. The solution is too
> invasive for my taste. Another approach might be to add new pattern elements
> FC_FAMILY_HASH, etc, that retain the (corresponding type of) hash of the
> string. It will be equally intrusive I'm afraid. But yeah, definitely
> something to pursue and see if better implementation is possible. Definitely
> biggest waste of time in current FontConfig.  Side note: if we go all the
> way to keep hashes of family names, we might end up wanting to use a
> hashtable for FcFontMatch...
> 
> * 3.4.3 Reducing heap allocations
> -> Not clear that is measurable. But sure, is not intrusive. I'm open to.
> 
> * 3.4.4 Refactoring FcStrCaseWalkerNext
> -> Sounds fine. We might not need to triplicate it, maybe just inline that
> function.
> 
> * 3.4.5 Micro-optimization in FcCompareValueList
> -> I have a hard time believing the 38% speedup claimed. If it has
> measurable impact, sounds good to me.
> 
> * 3.4.6 Reduce FcObjectFromName call amount
> -> Not sure it's worth it; has no measurable impact.
> 
> * 3.4.7 Hint branch predictor
> -> Good to take, since it's not intrusive. We do these in other libraries.
> 
> Anyway, I wish he had done his work in collaboration with us so we could
> have discussed and integrated this upstream without pain.  Thanks for
> bringing up.

Not a problem! At present moment, we are narrowing down the rebase area - in similar of your logic to include the most important and significant optimisations. Looking forward to future work on the upstream!

On a side note, I tried to generate a .pot file so we could start working on a zh_CN translation for Fontconfig... I ended up with no .pot file at all with the command below...

cd po/ && xgettext --files-from=POTFILES.in --directory=.. --output=messages.pot

If not, when are you expecting translation works to be ready to start?
Comment 9 Akira TAGOH 2018-02-15 03:01:44 UTC
will read the whole comments later carefully but just a quick comment for:

(In reply to Mingcong Bai from comment #8)
> On a side note, I tried to generate a .pot file so we could start working on
> a zh_CN translation for Fontconfig... I ended up with no .pot file at all
> with the command below...
> 
> cd po/ && xgettext --files-from=POTFILES.in --directory=..
> --output=messages.pot
> 
> If not, when are you expecting translation works to be ready to start?

simply try to run "make update-po" on both po and po-conf. you'll get POT generated once make finished.
Comment 10 Mingcong Bai 2018-02-15 03:07:36 UTC
(In reply to Akira TAGOH from comment #9)
> will read the whole comments later carefully but just a quick comment for:
> 
> (In reply to Mingcong Bai from comment #8)
> > On a side note, I tried to generate a .pot file so we could start working on
> > a zh_CN translation for Fontconfig... I ended up with no .pot file at all
> > with the command below...
> > 
> > cd po/ && xgettext --files-from=POTFILES.in --directory=..
> > --output=messages.pot
> > 
> > If not, when are you expecting translation works to be ready to start?
> 
> simply try to run "make update-po" on both po and po-conf. you'll get POT
> generated once make finished.

That worked, thanks!
Comment 11 Mingcong Bai 2018-02-16 06:46:22 UTC
(Knowing this is not going to be too relevant to the final upstreamed implementation, but for the sake of transparency...)

We have rebased our code upon 2.12.93, the code builds and works correctly.
Comment 12 Behdad Esfahbod 2018-02-16 18:22:24 UTC
(In reply to Mingcong Bai from comment #11)
> (Knowing this is not going to be too relevant to the final upstreamed
> implementation, but for the sake of transparency...)
> 
> We have rebased our code upon 2.12.93, the code builds and works correctly.

Oh that definitely helps!
Comment 13 Mingye Wang (Arthur2e5) 2018-02-16 23:36:54 UTC
We are looking at a problem introduced by srb's patches on 2.12.6 and 2.12.93 that causes synaptic's UI font to be serif only under sudo on our distro. It appears that the "Match Pattern" has a way longer family list when calling through sudo. We bisected the change to the FcFontSetMatchInternal optimization commit.

I am asking Bai to get the match pattern for an unpatched version so we can find out whether it's caused by something like a change in tie-breaking behavior (weird) or by something outside of FcFontSetMatchInternal that changes the match pattern (wild). 

* * *

I put back some FC_DEBUG functionalities for FcFontSetMatchInternal so that MATCH (1) works roughly as the same as before without the best score, and MATCHV (2) shows font indexes (like before, but without set barriers) and each priority rounds and the process of bitset ellimination after each round. It can get a bit too verbose when mixed with the individual subscores and their operands...

MATCH2 (4096) is still missing. Looks like I can just paste it back.
Comment 14 Behdad Esfahbod 2018-02-18 20:27:46 UTC
Where's your git tree?
Comment 15 Mingcong Bai 2018-02-19 07:56:27 UTC
(In reply to Behdad Esfahbod from comment #14)
> Where's your git tree?

Sorry! I thought I've put it up here - https://github.com/AOSC-Dev/fontconfig.
Comment 16 Mingye Wang (Arthur2e5) 2018-02-22 00:10:54 UTC
The bug is now fixed in our rebased version. The strong/weak score selection in michalsrb's optimized FcFontSetMatchInternal takes the first element of pattern's valuelist's strength; it should instead decide by (matcher->strong == priority).
Comment 17 Daniel Stone 2018-08-20 21:55:32 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/fontconfig/fontconfig/issues/103


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.