Cleanup: Store BLF Glyph cache with C++ Vector, use std::mutex #118222

This works great. And I can see the point of it being much simpler than mine. At the time I liked the idea of making it more object-y, but in hindsight it was a bit weird to make a new object just to manage the cache. This is a lot easier to follow.

Love that use of std::lock_guard - that is pretty cool.

One change, that I could just do afterward, is that blf_glyph_cache_find doesn't need that size argument as it is only ever called with font->size anyway. I should have changed that years ago.

I realize that a large part of this PR is as a teaching moment for me, to show a simpler alternative to mine, and to show me what we consider best practices. Of your changes everything makes perfect sense, except for the uses of unique_ptr so this is definitely something I have to learn more about.

A very obvious next step here seems to be replacement of GlyphCacheBLF->bucket, which is an array of 257 ListBases that act as a hash table. I did try replacing this with what I think is an obvious choice, std::unordered_map. This seemed to work okay when I just had it storing pointers to GlyphBLF, but I had no luck when I tried storing unique_ptr to them instead. Perhaps I could try again after this PR goes in and you could assist if I run into trouble again?

@HooglyBoogly Sorry for the delay in looking at this in detail. This works great. And I can see the point of it being much simpler than mine. At the time I liked the idea of making it more object-y, but in hindsight it was a bit weird to make a new object just to manage the cache. This is a lot easier to follow. Love that use of std::lock_guard - that is pretty cool. One change, that I could just do afterward, is that `blf_glyph_cache_find` doesn't need that `size` argument as it is only ever called with font->size anyway. I should have changed that years ago. I realize that a large part of this PR is as a teaching moment for me, to show a simpler alternative to mine, and to show me what we consider best practices. Of your changes everything makes perfect sense, except for the uses of `unique_ptr` so this is definitely something I have to learn more about. A very obvious next step here seems to be replacement of GlyphCacheBLF->bucket, which is an array of 257 ListBases that act as a hash table. I did try replacing this with what I think is an obvious choice, std::unordered_map. This seemed to work okay when I just had it storing pointers to GlyphBLF, but I had no luck when I tried storing `unique_ptr` to them instead. Perhaps I could try again after this PR goes in and you could assist if I run into trouble again?

Harley Acheson approved these changes 2024-02-18 03:31:06 +01:00

Hans Goudey added 1 commit 2024-02-19 02:17:02 +01:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

4fee33d770 Merge branch 'main' into cleanup-glyph-cache-vector

Hans Goudey commented

2024-02-19 02:17:14 +01:00

@blender-bot build

Hans Goudey commented

2024-02-19 03:30:28 +01:00

Thanks for checking it out.

Of your changes everything makes perfect sense, except for the uses of unique_ptr so this is definitely something I have to learn more about.

Let me know if you have any questions. Basically it's just nice that when we use this we don't have to worry about memory leaks or manually freeing things.

A very obvious next step here seems to be replacement of GlyphCacheBLF->bucket, which is an array of 257 ListBases that act as a hash table. I did try replacing this with what I think is an obvious choice, std::unordered_map

Yeah, I'd be happy to help with that. Indeed, that could be so much nicer. We do have Map as a better (faster) option than std::unordered_map BTW.

Thanks for checking it out. > Of your changes everything makes perfect sense, except for the uses of unique_ptr so this is definitely something I have to learn more about. Let me know if you have any questions. Basically it's just nice that when we use this we don't have to worry about memory leaks or manually freeing things. >A very obvious next step here seems to be replacement of GlyphCacheBLF->bucket, which is an array of 257 ListBases that act as a hash table. I did try replacing this with what I think is an obvious choice, std::unordered_map Yeah, I'd be happy to help with that. Indeed, that could be so much nicer. We do have `Map` as a better (faster) option than `std::unordered_map` BTW.

Hans Goudey merged commit fb8f9deaab into main

2024-02-19 03:34:44 +01:00

Hans Goudey referenced this issue from a commit

2024-02-19 03:34:46 +01:00

Cleanup: Store BLF Glyph cache with C++ Vector, use std::mutex

Hans Goudey deleted branch cleanup-glyph-cache-vector

2024-02-19 03:34:47 +01:00

Jeroen Bakker referenced this issue from a commit

2024-02-19 08:10:15 +01:00

Cleanup: Store BLF Glyph cache with C++ Vector, use std::mutex

Harley Acheson referenced this pull request

2024-02-20 01:43:38 +01:00