Another great bundle of messy stuff #1257

NiLuJe · 2020-12-11T00:15:18Z

Companion to koreader/crengine#399

Much like over there, I intend to somewhat cleanup the history before merge.

CBB:
- Reformat in a C99 fashion (tighter scope, restricted pointers).
  (This makes the diff a horrible mess to read, so, don't, and hope there aren't any more typos than those I already caught ;D).
- Made some helper macros available in the header (e.g., GET_BB_*).
- Very few logic changes, but a few which'll probably get back to me later :D.
- Update the BlitBuffer structs to use unsigned types, and made the fact that stride is a byte count explicit by using a size_t.
- Add support for straight-alpha blitFrom (useful for NanoSVG, which spits out straight alpha, unlike MµPDF).
- Don't clobber the alpha byte in paintRect & fill (c.f., the tail end of Icon update using MaterialDesignLight koreader#6937)
LBB:
- Minor updates for feature-parity w/ CBB.
- Fair warning about the stride is now size_t thing: a Lua number isn't precise enough to fit the full range of an uint64_t, so, on platforms where size_t is 64bit, it's not automatically coerced to a Lua number. The frontend companion PR takes care of the very few cases w<here this matters.
- Fix a few instances where the invert flag was mishandled.
Add ZSTD and an FFI binding.
- ZSTD is now used by CRe to compress its cache
- And it's also used by BookInfo to compress the thumbnails
- BookInfo's now unused xutil has been moved to ffi/zlib
Rejig our understanding and usage of the __gc metamethod: it doesn't run on plain tables in Lua 5.1, only on userdata. In LuaJIT, it's also usable on cdata metatypes, and cdata finalizers can also be setup explicitly.
Convert stuff to the sanest solution in each case, and, in cases where it's not possible, use the hacky workaround of introducing an empty newproxy userdata in the mix, via ffi/__gc.
This finally solves a number of puzzling code comments that were wondering why the hell our __gc methods were never called (hint: because it's not supported, or, in a few cases it's not supported and they were misnamed anyway ^^).
Tweak lj-sqlite3 to stop coercing BLOBs as Lua strings. Instead return a raw pointer and a byte count, like the SQLite C API. This allows avoiding a few useless copies in BookInfo, our sole user of BLOB. It also just plain makes sense, because if I wanted strings, I'd be using a TEXT column ;).
Use proper symbol visibility discipline for our Lua/C modules, much like was done in nnsvg. This avoids leaking a ton of useless symbols, and makes the dynamic loader's job that much easier.
Update the stbiw version shipped by NanoSVG to the latest one (only used by a CRe helper function).
Fun Valgrind experiments in cre.cpp if DEBUG_CRENGINE is set ;p.

This change is

poire-z · 2020-12-11T00:35:06Z

Does that mean KOReader doesn't leak memory anymore ?! Well done ! :)

NiLuJe · 2020-12-11T00:38:55Z

In practice, it showed that most of our manual cleanup efforts did the job mostly correctly ;).

(Hence the extra-shitty double-free guards in KOptContext, for instance ^^).

As far as BB is concerned, I haven't really caught much that was missed, maybe a few tiny BBs during startup.

Incidentally, that's the reason for a few finalizers using a dedicated function when they just call our old manual finalizer: that was to debug which is which when they trigger ;).

NiLuJe · 2020-12-11T00:42:51Z

I'm still utterly puzzled by where roughly 8MB of memory go each time a CRe document is opened (the only thing I can say is that's it's probably CRe logic unrelated to BB/DrawBuf, because it's not affected by the target buffer size), but the PDF stuff seem pretty solid, with clearly visible chunks of memory being reclaimed at runtime in a single document.

DjVu appears to be a fairly bad student, and will eat a metric shitton of RAM each time you open a new page, and not reclaim any of it on close.

poire-z · 2020-12-11T00:54:22Z

I'm still utterly puzzled by where roughly 8MB of memory go each time a CRe document is opened

I haven't been seeing that (looking at the footer Memory info).
I can see it increase sometimes (on load, as you browse pages), but I sometimes also see decreases.
Switching now between 2 small Wikipedia EPUBs, I see 29M > 30 > 29 > 30 > 29 > 30.

What I see is it raises a log when you do a dict lookup, show history,... and then stay there.
(Some previous comments of mine about it at koreader/koreader#5998 (comment))

NiLuJe · 2020-12-11T01:08:08Z

I'm still utterly puzzled by where roughly 8MB of memory go each time a CRe document is opened

I haven't been seeing that (looking at the footer Memory info).
I can see it increase sometimes (on load, as you browse pages), but I sometimes also see decreases.
Switching now between 2 small Wikipedia EPUBs, I see 29M > 30 > 29 > 30 > 29 > 30.

What I see is it raises a log when you do a dict lookup, show history,... and then stay there.
(Some previous comments of mine about it at koreader/koreader#5998 (comment))

Haven't tried a CRe -> CRe swap, so this may not actually all be at CRe's feet, but doing a CRe -> FM -> CRe nets me a ~4 to 5MB increase each time on Kobo.

NiLuJe · 2020-12-11T01:12:17Z

(A CRe -> CRe swap doesn't make a lick of difference over here, both with or without my mess applied, similar behavior between the H2O/Forma/emu ^^).

poire-z · 2020-12-11T08:03:28Z

A CRe -> CRe swap doesn't make a lick of difference

A difference vs what ? :) You mean you get the same memory increase as CRe>FM>CRe ? Or that you get the same memory usage (no increase) ?

Frenzie

note to self, read through blitbuffer and zstd.lua later

poire-z

Looks fine, no real comment.

poire-z · 2020-12-11T12:31:58Z

thirdparty/kpvcrlib/CMakeLists.txt

@@ -20,15 +20,17 @@ set(ANTIWORD_INCLUDE_DIR ${CR_3RDPARTY_DIR}/antiword)
 set(CHM_INCLUDE_DIRS ${CR_3RDPARTY_DIR}/chmlib/src)
 set(CREGINE_INCLUDE_DIR ${CRE_DIR}/include)

+assert_var_defined(JCONFIG_INCLUDE_DIR)


Any reason for you to move this at top ? Looks a bit related to JPEGLIB - or may be not, dunno :)

It was to keep the same order as the order in which they're defined/used below ;).

poire-z · 2020-12-11T12:34:44Z

xtext.cpp

@@ -360,7 +360,7 @@ class XText {
        if (m_bidi_ctypes) { free(m_bidi_ctypes); m_bidi_ctypes = NULL; }
        if (m_bidi_btypes) { free(m_bidi_btypes); m_bidi_btypes = NULL; }
        if (m_bidi_levels) { free(m_bidi_levels); m_bidi_levels = NULL; }
-        if (m_lang)        { delete m_lang;       m_lang = NULL; }
+        if (m_lang)        { delete[] m_lang;     m_lang = NULL; }


Does this change anything in any way in what the compiler would do ? Or it's just for sake of correctness?

It deletes the entire array of characters instead of the first one. Until now the memory of the rest of characters was leaked on each deallocation

I get the idea.
But it's one single thing I created/allocated with m_lang = new char[strlen(lang)+1]; - so I find it strange the compiler would just delete the first char (a fragment of that thing, while it knows how much was allocated). It's my idea the compiler would do the right thing, but I don't know him personally :)

I don't know him personally :)

Me neither 😄

But C++ is a humongous beast with operator overloading and other stuff and this specific behaviour is forbidden. See https://isocpp.org/wiki/faq/freestore-mgmt#delete-array-built-ins

It's apparently mostly harmless on Linux, but, that is admittedly an implementation detail, and Valgrind was certainly unhappy about it ;).

EDIT: Oh, read through the link, yeah, operator overloads would certainly complicate the matter ^^.

NiLuJe · 2020-12-11T15:01:55Z

A CRe -> CRe swap doesn't make a lick of difference

A difference vs what ? :) You mean you get the same memory increase as CRe>FM>CRe ? Or that you get the same memory usage (no increase) ?

Yep, same increase.

Checking statm manually, the increase is actually in two steps: on close, and then on open.

c.f.,

emu:

echo $(( $(cut -f2 -d' ' /proc/$(pidof luajit | cut -f2 -d' ')/statm) * 4096 / 1024 / 1024. ))

Kobo:

echo $(( $(cut -f2 -d' ' /proc/$(pidof reader.lua | cut -f2 -d' ')/statm) * 4096 / 1024 / 1024. ))

NiLuJe · 2020-12-11T15:02:26Z

I also think I have a vague idea of why djvu is stuffing itself like crazy, I'll see if I can tame it a bit.

EDIT: Nope that's just the Cache's watermark being high ;).

NiLuJe · 2020-12-11T15:09:10Z

Checking statm manually, the increase is actually in two steps: on close, and then on open.

Slightly more subtle than that, which means possibly not all-that-CRe-related ;).

(On the H2O w/ this applied)

Restart KOReader: 19.921875
Open folder with single book: 23.6328125
Tap book: 38.68359375
Open top menu (on search tab): 43.12890625 <- Oh, hi, 4MB I was talking about ;p.
Back to FM: 40.7109375
Tap book: 44.76171875
Menu: 47.63671875
Back to FM: 45.5546875
Tap book: 49.28125
Menu: 52.49609375
Back to FM: 49.51171875
Tap book: 53.36328125
Menu: 56.609375
FM: 53.6171875

etc. ;).

Soooo, probably not so much CRe's fault ;p.

(Whether xtext is enabled or not has not much bearing on the results, FWIW).

zwim · 2020-12-11T20:03:54Z

Horray. I admire zstd. I wIll test this bunch of commit tomorrow. +5 from my side :))

NiLuJe · 2020-12-12T00:40:32Z

@poire-z: FWIW, now that I finally have a somewhat readable Valgrind output (hi, koreader/koreader#6980), most, if not all, of the "still reachable" stuff is from FT/HB.

It's a bit hard to dig into that, because AFAICT, LuaJIT doesn't unload packages on exit (and we don't really have teardown facilities in place anyway), so it's hard to judge which of those pointers are actually expected to stay alive (i.e., cached stuff).

NiLuJe

Reviewed 30 of 30 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @poire-z)

NiLuJe · 2020-12-14T22:09:24Z

History cleaned up (use a merge commit to merge); detailed history archived in https://github.com/NiLuJe/koreader-base/tree/cbb-c99_archive

poire-z · 2020-12-14T22:42:18Z

316a72d feels like it doesn't deserve its own commit - and could be squased in the main commit(s) that touched these parts.

use a merge commit to merge

Isn't "Rebase and merge" cleaner ? (no strange "Merge pull request..." additional commit that seems to redo what the previous do).
Or there's something I don't get about that "Create a merge commit" - what is it ? :)

NiLuJe · 2020-12-14T22:52:33Z

316a72d feels like it doesn't deserve its own commit - and could be squased in the main commit(s) that touched these parts.

Agreed, but it was simply too hellish to re-order in terms of merge conflicts ;).

Or there's something I don't get about that "Create a merge commit" - what is it ? :)

It makes it clear that a series of commits belonged to a single PR (besides the tiny, tiny mention about that in GitHub's UI when looking at individual commits).

I'm not generally a fan of it, but when the series is as "messy" as these, it kinda helps a bit?

poire-z · 2020-12-14T23:34:32Z

It makes it clear that a series of commits belonged to a single PR
I'm not generally a fan of it, but when the series is as "messy" as these, it kinda helps a bit?

A tiny little wee bit, ok :)

There's another little thing I don't like much, it's the commit timestamps. I'd rather have them all the date of the merge, that a mix of misordered date from the last 2 weeks (which may make following the changes a bit confusing).
On my side, when there's work spanning multiple days, or when re-ordering/squashing/cleaning, I just do before pushing:
git rebase --force-rebase --exec "sleep 2" --exec "git commit --amend --date=now" master

NiLuJe · 2020-12-15T00:14:30Z

git rebase --ignore-date ;o).

poire-z · 2020-12-15T00:16:51Z

I think I needed the sleep 2, or sometimes, Github would not show them in my original order (they may be in the correct order once merged, dunno).

NiLuJe · 2020-12-15T15:07:36Z

Okay, Busted apparently has a really hard time dealing with cdata finalizers (even when running Busted master, which supposedly does it better), so, ensure the tests properly cleanup behind themselves.

Also, finally fix the KOptContext language thingy so that it stops randomly failing because that's not how you copy a C string ;p.

* Tighter scope, const, restricted pointers * Avoid array indexing where possible * Use unsigned data types for dimensions/sizes That's really what the code expects, and everything is already bounds-checked on the Lua side. * Resync dither_o8x8 w/ FBInk * In the few places that weren't already doing so, cadge a few shifts by relying on pointer arithmetic rules, much like BB_GET_PIXEL * Implement ditheralphablitFrom (straight alpha) * Move the helper macros to the header, it's useful for C consumers * ffi/blitbuffer: Clear up a c/p snafu w/ invertBlitFrom * ffi/blitbuffer: Good-bye, lovingly handcrafted BMP encoding ;)

* Also, make sure make dist-clean works in front * nnsvg: Enable the BB type check, now that it's available

…usly outdated.

* Build CRe w/ zstd support * Other minor CRe build cleanups after koreader/crengine#399 * Move zlib FFI from xutil in the coverbrowser plugin to a dedicated module * As far as our wrapper goes, the ZSTD one differs from zlib in that it has switched to explicit memory allocation: Instead of taking/returning a Lua string copy, take/return a cdata ptr, size tuple. (Technically, void *, size_t)

* When *getting* a BLOB, return instead a tagged table, with the first elements being the (void) pointer, and the second the amount of data in bytes. * When *setting* a blob, free the original pointer: SQL_TRANSIENT means SQLite's just made a copy of it.

* Allow reusing a zstd decompression context * Handle the context's lifetime via a cdata finalizer

(c.f., comments in ffi/blitbuffer) * BlitBuffer: handle it via an explicit cdata finalizer in setAllocated Move BlitBuffer memory management to FFI cdata finalizer * KoptContext: put the __gc metamethod in the right place, and add a bunch of double-free guards now that it actually triggers. * Made NULL pointer checks more explicit: ctypes are sticky; assigning nil to a cdata<ptr> makes that a NULL ptr, not a plain lua nil, meaning if foo is *true* for a foo cdata NULL ptr. (This is clearer in vanilla libffi, where NULL/nullptr is usually a dedicated constant, e.g., ffi.NULL). * MuPDF: via explicit cdata finalizers * Comment out the useless __gc methods in ffi/pic * Import https://github.com/katlogic/__gc To make the very last few things that tried to do this on plain tables in front actually work...

* Don't leave a dangling dom_doc pointer * When debugging, implement a _fini to make Valgrind happier about CRe This'll run on library unload (i.e., on exit).

Fix LuaCheck warnings: ffi.gc does return the cdata unchanged We apparently can't use a local function as a finalizer

NiLuJe

Reviewed 11 of 11 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @poire-z)

Also resync GH workflow with front

ensure proper teardowns are put into place, because Busted can't deal with cdata finalizers sanely. Fix KOptContext language copy: Make sure it's NUL terminated, it's a C string.

* Handle CFLAGS & LDFLAGS via env vars, it's less likely to implode in fun and interesting ways down the line... * Refresh ZSTD ffi wrapper for v1.4.7

* In the C variant, pack the pixels ourselves instead of going through the Color structs, as it generates shorter code. (We probably should make that clearer like with the union trickery in FBInk). * Fix a number of inconsistencies in how the invert flag was sometimes mishandled in the Lua blitter (it's still completely ignored by the C blitter). * Handle inversion in Lua fill * Fix a bug where, in most blend methods, the inversion flag was only honored when blending, and not for the fully opaque fast path...

NiLuJe · 2020-12-19T01:02:04Z

(Latest history archived in https://github.com/NiLuJe/koreader-base/tree/cbb-c99-front_archive_r2 (nice typo!) :D).

NiLuJe mentioned this pull request Dec 11, 2020

The final bit of my great bundle of messy stuff koreader/koreader#6976

Merged

Frenzie reviewed Dec 11, 2020

View reviewed changes

poire-z approved these changes Dec 11, 2020

View reviewed changes

poire-z reviewed Dec 11, 2020

View reviewed changes

NiLuJe commented Dec 14, 2020

View reviewed changes

NiLuJe force-pushed the cbb-c99 branch from 922e791 to 5827f6e Compare December 14, 2020 22:08

NiLuJe force-pushed the cbb-c99 branch from 5827f6e to 6100f50 Compare December 14, 2020 22:50

NiLuJe force-pushed the cbb-c99 branch from 6100f50 to 921e5a3 Compare December 15, 2020 00:14

NiLuJe mentioned this pull request Dec 15, 2020

Icon update using MaterialDesignLight koreader/koreader#6937

Merged

NiLuJe added 2 commits December 15, 2020 20:40

Build all our custom Lua/C & Lua/FFI modules w/ -fvisibility=hidden

7da1589

* Also, make sure make dist-clean works in front * nnsvg: Enable the BB type check, now that it's available

NiLuJe added 8 commits December 15, 2020 20:40

Replace NanoSVG's bundled stbi lib with a version that is not hilario…

446684d

…usly outdated.

lj-sqlite3 & zstd: Better asserts

db8bf32

* Allow reusing a zstd decompression context * Handle the context's lifetime via a cdata finalizer

xtext: That's an array of char, use delete[], not delete

4ddf1a1

CRe: Minor cleanups

5498d40

* Don't leave a dangling dom_doc pointer * When debugging, implement a _fini to make Valgrind happier about CRe This'll run on library unload (i.e., on exit).

Final review pass

b0d25bf

Fix LuaCheck warnings: ffi.gc does return the cdata unchanged We apparently can't use a local function as a finalizer

NiLuJe force-pushed the cbb-c99 branch from fe81172 to 8fa107b Compare December 15, 2020 19:41

NiLuJe commented Dec 15, 2020

View reviewed changes

NiLuJe mentioned this pull request Dec 18, 2020

Update UI layout code for use of SVG icons koreader/koreader#6977

Merged

NiLuJe added 5 commits December 19, 2020 01:27

Bump CRe to pickup koreader/crengine#399

5582d85

CI: Update to Clang 11 & Busted 2.0.0

788fdbc

Also resync GH workflow with front

Fix MuPDF & KOptContext tests:

2b9af95

ensure proper teardowns are put into place, because Busted can't deal with cdata finalizers sanely. Fix KOptContext language copy: Make sure it's NUL terminated, it's a C string.

Update ZSTD to 1.4.7

456ebd6

* Handle CFLAGS & LDFLAGS via env vars, it's less likely to implode in fun and interesting ways down the line... * Refresh ZSTD ffi wrapper for v1.4.7

NiLuJe force-pushed the cbb-c99 branch from 8fa107b to 961bb78 Compare December 19, 2020 00:31

NiLuJe merged commit b00f7f1 into koreader:master Dec 19, 2020

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Dec 19, 2020

Bump base to pickup koreader/koreader-base#1257

9e70bf8

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Dec 19, 2020

Bump base to pickup koreader/koreader-base#1257

79dd3f3

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Dec 19, 2020

Bump base to pickup koreader/koreader-base#1257

54186ad

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Dec 19, 2020

Bump base to pickup koreader/koreader-base#1257

0ab4023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Another great bundle of messy stuff #1257

Another great bundle of messy stuff #1257

NiLuJe commented Dec 11, 2020 •

edited

Loading

poire-z commented Dec 11, 2020

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

poire-z commented Dec 11, 2020

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

poire-z commented Dec 11, 2020

Frenzie left a comment

poire-z left a comment

poire-z Dec 11, 2020

NiLuJe Dec 11, 2020

poire-z Dec 11, 2020

pazos Dec 11, 2020

poire-z Dec 11, 2020

pazos Dec 11, 2020

NiLuJe Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

zwim commented Dec 11, 2020

NiLuJe commented Dec 12, 2020

NiLuJe left a comment

NiLuJe commented Dec 14, 2020

poire-z commented Dec 14, 2020

NiLuJe commented Dec 14, 2020 •

edited

Loading

poire-z commented Dec 14, 2020

NiLuJe commented Dec 15, 2020

poire-z commented Dec 15, 2020

NiLuJe commented Dec 15, 2020

NiLuJe left a comment

NiLuJe commented Dec 19, 2020

Another great bundle of messy stuff #1257

Another great bundle of messy stuff #1257

Conversation

NiLuJe commented Dec 11, 2020 • edited Loading

poire-z commented Dec 11, 2020

NiLuJe commented Dec 11, 2020 • edited Loading

NiLuJe commented Dec 11, 2020 • edited Loading

poire-z commented Dec 11, 2020

NiLuJe commented Dec 11, 2020 • edited Loading

NiLuJe commented Dec 11, 2020 • edited Loading

poire-z commented Dec 11, 2020

Frenzie left a comment

Choose a reason for hiding this comment

poire-z left a comment

Choose a reason for hiding this comment

poire-z Dec 11, 2020

Choose a reason for hiding this comment

NiLuJe Dec 11, 2020

Choose a reason for hiding this comment

poire-z Dec 11, 2020

Choose a reason for hiding this comment

pazos Dec 11, 2020

Choose a reason for hiding this comment

poire-z Dec 11, 2020

Choose a reason for hiding this comment

pazos Dec 11, 2020

Choose a reason for hiding this comment

NiLuJe Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

NiLuJe commented Dec 11, 2020 • edited Loading

NiLuJe commented Dec 11, 2020 • edited Loading

NiLuJe commented Dec 11, 2020 • edited Loading

zwim commented Dec 11, 2020

NiLuJe commented Dec 12, 2020

NiLuJe left a comment

Choose a reason for hiding this comment

NiLuJe commented Dec 14, 2020

poire-z commented Dec 14, 2020

NiLuJe commented Dec 14, 2020 • edited Loading

poire-z commented Dec 14, 2020

NiLuJe commented Dec 15, 2020

poire-z commented Dec 15, 2020

NiLuJe commented Dec 15, 2020

NiLuJe left a comment

Choose a reason for hiding this comment

NiLuJe commented Dec 19, 2020

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 11, 2020 •

edited

Loading

NiLuJe commented Dec 14, 2020 •

edited

Loading