Feature: GDI font cache

[razielanarki]

extracted the font cache feature as a separate PR from #147

[TheQwertiest]

Thanks for separating the PR =)
But first things first: did you measure the performance impact? I.e. is there actually any notable difference?

[razielanarki]

well on one hand yes: the (limited) gdi handles used is much lower, as the code creates shared_hfont-s for the "same" logfonts
(see: gdi_font_cache.h: std::hash<LOGFONTW> and also: gdi_font_cache.cpp: Normalize(..), the latter called from fb_tooltip.cpp and gdi_font.cpp),

on top of that, the JsGdiFont getters are also setters (with some extras), making api usage much easier.

however as to freeing JsGdiFont-s and releasing all shared_hfont-s: i've yet to get the gc kick in (using the default settings), and didn't really wanted to muck around in js:gc stuff until esr91 (reloading the test script panel seemed to release the "stuck(?)" JsGdiFont instances)
maybe JsGdiFont's could be eagerly collected?

[razielanarki]

(also, on a side note: i've tried to add the debug .cache getter as a static property of GdiFont, but failed, that's why it's available on every instance, heh)

[razielanarki]

note2:
as per CreateFontW docs, i've allowed 0 (seems to get a reasonable default size, likely 11px or 12px) and negative font sizes (but flipped sign in contrast to the ms docs: to keep compatibility with the current impl,, while also allowing for selecting fonts accodring to "line-height" pixels too (could help with layouts) -> the aforementioned "Normalize" function caches only one instance for eg -16px/ 0px /+12px )

note3: using a gibberish font name like "!INVALID" seems to get the system default font (Segoe) from the gdi font mapper, but keeping the gibberish name.

[razielanarki]

after reading up a bit on js api / gc stuff, it seems that calling the c++ destructor only happens on gc finalization, and does not on "delete"ing / overwriting the GdiFont variable (as there's no js/es6 equivalent of a destructor callback), unless maybe some sort of non-obvious weakref trickery or is involved, so the best case for now is the lowered gdi handle count (normalized/shared+reused across panel instances in a foobar instance)

[TheQwertiest]

A few issues:

• This PR still contains multiple features and should be further divided: addition of negative line-height font size, additional font write methods and font caching.
• Regarding negative 'line-height' font size: zero font size should lead to a hard error, using unknown default implementation-specific size is kinda meh.
• Regarding additional font write methods: this encourages user to create new font objects instead of reusing them, which I don't think is a good practice (since it might lead to negative performance impact).
• Regarding font caching:
  • The proper way would be to use LRU cache as to avoid destroying fonts that are frequently used.
  • Font cache should be an object instead of a namespace with functions: this will allow for better management of it's lifetime.
• Removal of GdiFont eager constructor: are you sure that there would be no performance impact? IMO, it would be better just to cache it alongside HFONT.
• Custom hash implementation: I don't think this is needed: usually there is not a lot of fonts even in most heavy themes, so even if there are some collisions (default hashing algorithm is good enough for most cases) it would not impact performance in any noticeable way. The proper way of implementing custom hash for an object is by implementing a hash method AND by implementing a full operator== that compares all object fields (operator== is called only when there is a hash collision). See https://en.cppreference.com/w/cpp/container/unordered_map/unordered_map for example.
• Code style:
  • Please don't use SAL unless it's really necessary (e.g. like here: https://github.com/TheQwertiest/fb2k_utils/blob/8afa595ecfda24a5d82089ae69cd7d6f356d9da6/src/qwr/qwr_exception.h#L34)
  • Please don't use using namespace in headers.
  • All contents of if should be braced even if it is a single line.

[TheQwertiest]

It should be mentioned though, that I really appreciate your contributions - it's never easy working with someone else's codebase.

PS: Concept stuff is neat, I should really spend some time familiarizing myself with it =)

[razielanarki]

noted, will amend :)

• additional font write methods

if you mean dwrite, that's not included (yet), as the "drawtext/drawstring compatible" dwrite methods are in terrible need of refactoring/rethinking.

• Regarding negative 'line-height' font size: zero font size should lead to a hard error, using unknown default implementation-specific size is kinda meh.

the default font size is always "-16" (depending on the font face, this means 12-14px actual size) according to my experiments (seems like the wine/reactos sources / tests confirm this, but i can't find the exact check myself in native ms dll-s using disassembly)

• The proper way would be to use LRU cache as to avoid destroying fonts that are frequently used.

currently it's just a refcounted weakptr -> shared ptr cache, without any extra mechanisms, implementing an lru seemed like overkill, as reuse already helps reducing the gdi handle counts a lot.

• Custom hash implementation:

the custom hash implementation is a bit of a micro-optimization story, as when "packing" the properties cached (and using one call to hash_combine vs one call for each property), afaik the "simpler" boost one may already lead to collisions than the alternative (which is a simple attempt to spread the bits more evenly than the former, the algo is explained on the link in the comment (which... became dead since apparently)... so here's a sw answer based on it : https://stackoverflow.com/a/50978188/1326931 )

[TheQwertiest]

if you mean dwrite

I mean addition of Set* methods

implementing an lru seemed like overkill

LRU cache is not that complex (I think it would be around 100 LOC for a basic generic implementation), but it will greatly improve the scenario where user creates the same Font object in every on_paint.

the custom hash implementation is a bit of a micro-optimization story

I do understand that std hash algorithm might not be the most optimal implementation, I just don't think that algorithm improvement brings anything noticeable to the table: even if we have 100 fonts and they will all have the collision (which is actually impossible), there will be most likely no performance impact at all, due to data locality. Such things usually only noticeable on HUGE number of keys (e.g. 10k).

[TheQwertiest]

Such things usually only noticeable on HUGE number of keys (e.g. 10k).

Real-life scenario: in our company we have a lot of 1mil+ sized unordered maps that use std hash. A lot of them are in pretty hot loops. We have yet to encounter a single case where there was a noticeable performance loss caused by inefficiency of std::hash.

[razielanarki]

I mean addition of Set* methods

ah, those all call the "jsgdifont::reload" function, which tries to reuse an existing item from the cache if a matching one is already cached (the jsgdifont maintains a logfontw which is edited, and reloaded+normalized resulting in a possibly different shared_hfont ptr from the cache pool, when using the setters).
and actually this was one of the reasons from an api consumer pov as to why i even started implementing a cache. 👼

[razielanarki]

inefficiency of std::hash.

i'm not overriding std:hash in any way, i'm just defining a function to combine two such hashes, which is useful when hashing struct-s.
(errm, two functions: the one copied from the boost library, and the slightly optimized one linked before)

[razielanarki]

note: force-pushing because i've accidentally included stuff from #152

[TheQwertiest]

ah, those all call the "jsgdifont::reload" function...

My point still stands: font should be a read-only object - this is also the way font objects are treated in WinAPI and mainstream languages with built-in font objects as well (e.g. C#, Java). DWrite objects is a whole separate beast and I will probably make separate objects and methods for it anyway.

There are also two other points that were not addressed: LRU cache implementation and GdiPlus object caching.

[razielanarki]

LRU cache is not that complex (I think it would be around 100 LOC for a basic generic implementation), but it will greatly improve the scenario where user creates the same Font object in every on_paint.

that scenario is already handled: a shared_hfont wrapping the same HFONT should be returned from the cache as it is. (utilizing the internal reference counters of weak/shared_ptr-s)

implementing an LRU is not that hard, but i'm not so sure about deleting references to fonts which are supposedly still used somewhere :/

Removal of GdiFont eager constructor

hmm, looking at gdiplusfont.h (i missed this beforehand) it seems that what GDI+ does is: GetObject-s a LOGFONT from the HFONT, and uses that to create the GpFont it internally uses...

so, yes, there's improvements to be made in this regard. 👼

[razielanarki]

(rebased on merged master)

[razielanarki]

regarding GC: i think i'm found something in the comments js/Class.h#L490: link on UDN, but i'm not sure about the implementation... i'm about 99% sure i need some additional check, but i can't figure it out..

...could it be as simple as JS::RuntimeHeapIsCollecting(), or JS::ObjectIsMarkedGray( obj ) ?

i'm a bit stuck on this one... maybe you have some insight?

void JsGdiFont::Trace( JSTracer* trc, JSObject* obj )
{
    if ( trc->isMarkingTracer() && ( __SOME_ADDITIONAL_CHECK__ ))
    {
        auto pNative = static_cast<JsGdiFont*>( JS_GetPrivate( obj ) );
        pNative->font.reset();
    }
}