compile a static crengine library #1679
Conversation
benoit-pierre
force-pushed
the
pr/static_crengine_lib
branch
from
d6d9f8c to
2be832e
Compare
|
Oops, I forgot about the ugly duckling… (macOS) |
benoit-pierre
force-pushed
the
pr/static_crengine_lib
branch
from
2be832e to
9660111
Compare
|
Any noticable change to the (re)build time, when touching a single crengine source file? |
No. |
benoit-pierre
force-pushed
the
pr/static_crengine_lib
branch
from
9660111 to
9e5b9c2
Compare
Yes. |
benoit-pierre
force-pushed
the
pr/static_crengine_lib
branch
from
9e5b9c2 to
0d0947a
Compare
Update `CMakeLists.txt` tests for `KODEBUG` to match the makefiles: check for a non-empty `KODEBUG`, not for definition (`kodev` export an empty `KODEBUG` on release builds).
- using `$@` for some of those multiple-targets rules is a mistake (it's not going be the library name when triggered by a dependency on the include directory) - add missing targets (include directory, library names)
Generate a custom `crsetup.h`, so users of the library will be sure to use the correct (same) flags.
There's only one user: `libkoreader-cre.so`.
Set it to `inlineshidden`, since we compile a static library and don't need to export anything (all accesses go through `libkoreader-cre.so`).
benoit-pierre
force-pushed
the
pr/static_crengine_lib
branch
from
0d0947a to
f97de61
Compare
|
(Nearly random issue to post this;) A quick try at launching gdb via some script around I use for the main process failed to give me more info with these background processes. And not much realtime atm to dig into that. ps: Everything works: it's probably happening at teardown, and these subprocess explicitely are not terminated properly to not waste time. |
|
On a Kobo or the emulator? (Or both?) I don't recall seeing this otoh but I'll take a look later. |
|
Emulator, I haven't checked on my Kobo (where I don't even know if/where's the /var/log/messages equivalent :) |
|
Indeed, same here. |
|
Thanks. |
How recent are we talking? I can't reproduce. I don't think the static crengine library is to blame. I'd be more inclined to think it's an issue with one of the leak-on-exit fix combined with the (horrible?) use of fork to extract metadata in the background or do partial rendering. From atexit man page:
Particularly for the crash in the @poire-z: Can you run a |
Looking at my /var/log/messages, I didn't get them before I think It was the day before I pulled stuff and got your big base changes that needed me to kodev clean (and lost all my settings :/) and recompile everything. I've been a bit busy and not focused on KOReader these last weeks, so I haven't really followed what I picked and when I pulled - but I'm not lagging more than 2 or 3 days, so your commit from 3 weeks ago is probably not the culprit. Just tried reverting line 144
:( it was the cleverest idea I've ever had related to KOReader :/
I have no such segfault with 1) refresh cached book information on a book in history - neither 2) partial rerendering nor 3) wikipedia lookup and save as epub.
No time right now to get involved in that and rebuild the whole thing :/ |
|
By the way, to debug with gdb, use: And use |
Thread 5.1 "luajit" received signal SIGSEGV, Segmentation fault.
0x00007ffff7096f04 in LVRefCounter::getRefCount (this=0x58) at ../../subprojects/crengine/crengine/src/../include/lvref.h:71
71 int getRefCount() const { return refCount; }
(gdb) up
#1 0x00007ffff70a8a2d in LVProtectedFastRef<LVFont>::getRefCount (this=0x7ffff213c540)
at ../../subprojects/crengine/crengine/src/../include/lvref.h:336
336 int getRefCount() const { return _ptr->getRefCount(); }
(gdb) up
#2 0x00007ffff70984aa in LVFontCache::clear (this=0x7ffff213c430) at ../../subprojects/crengine/crengine/src/lvfntman.cpp:732
732 assert(fnt.isNull() || fnt.getRefCount() == 1);
(gdb) l
727 delete _registered_list.remove(i);
728 }
729 for (i = _instance_list.length(); i--; ) {
730 #ifndef NDEBUG
731 LVFontRef &fnt = _registered_list[i]->_fnt;
732 assert(fnt.isNull() || fnt.getRefCount() == 1);
733 #endif
734 delete _instance_list.remove(i);
735 }
736 }
(gdb) print fnt.isNull()
11/04/23-21:48:25 WARN PageBrowserWidget thumbnail deserialize() failed: malformed serialized data (unexpected end of buffer)
$1 = false
(gdb) print fnt
$2 = (LVFontRef &) @0x7ffff213c540: {_ptr = 0x50}:/ |
|
OK, tentative patch: base/cre.cpp | 7 +++++++
frontend/apps/reader/modules/readerthumbnail.lua | 3 +++
frontend/document/credocument.lua | 4 ++++
base/thirdparty/kpvcrlib/crengine/crengine/src/lvfntman.cpp | 2 +-
4 files changed, 15 insertions(+), 1 deletion(-)
diff --git i/base/cre.cpp w/base/cre.cpp
--- i/base/cre.cpp
+++ w/base/cre.cpp
@@ -764,6 +764,12 @@ static int isCacheFileStale(lua_State *L) {
return 1;
}
+static int setCacheFileStale(lua_State *L) {
+ CreDocument *doc = (CreDocument*) luaL_checkudata(L, 1, "credocument");
+ doc->dom_doc->setCacheFileStale(lua_toboolean(L, 2));
+ return 0;
+}
+
static int invalidateCacheFile(lua_State *L) {
CreDocument *doc = (CreDocument*) luaL_checkudata(L, 1, "credocument");
doc->dom_doc->invalidateCacheFile();
@@ -4023,6 +4029,7 @@ static const struct luaL_Reg credocument_meth[] = {
{"isBuiltDomStale", isBuiltDomStale},
{"hasCacheFile", hasCacheFile},
{"isCacheFileStale", isCacheFileStale},
+ {"setCacheFileStale", setCacheFileStale},
{"invalidateCacheFile", invalidateCacheFile},
{"getCacheFilePath", getCacheFilePath},
{"updateTocAndPageMap", updateTocAndPageMap},
diff --git i/frontend/apps/reader/modules/readerthumbnail.lua w/frontend/apps/reader/modules/readerthumbnail.lua
--- i/frontend/apps/reader/modules/readerthumbnail.lua
+++ w/frontend/apps/reader/modules/readerthumbnail.lua
@@ -319,6 +319,8 @@ end
function ReaderThumbnail:startTileGeneration(request)
local pid, parent_read_fd = ffiutil.runInSubProcess(function(pid, child_write_fd)
+ self.ui.document:setCallback()
+ self.ui.document:setCacheFileStale(false)
-- Get page image as if drawn on the screen
local bb = self:_getPageImage(request.page)
-- Scale it to fit in the requested size
@@ -337,6 +339,7 @@ function ReaderThumbnail:startTileGeneration(request)
-- bb:free() -- no need to spend time freeing, we're dying soon anyway!
ffiutil.writeToFD(child_write_fd, self.codec.serialize(tile:totable()), true)
+ self.ui.document:close()
end, true) -- with_pipe = true
if pid then
-- Store these in the request object itself
diff --git i/frontend/document/credocument.lua w/frontend/document/credocument.lua
--- i/frontend/document/credocument.lua
+++ w/frontend/document/credocument.lua
@@ -1447,6 +1447,10 @@ function CreDocument:isCacheFileStale()
return self._document:isCacheFileStale()
end
+function CreDocument:setCacheFileStale(stale)
+ return self._document:setCacheFileStale(stale)
+end
+
function CreDocument:invalidateCacheFile()
self._document:invalidateCacheFile()
end
diff --git i/base/thirdparty/kpvcrlib/crengine/crengine/src/lvfntman.cpp w/base/thirdparty/kpvcrlib/crengine/crengine/src/lvfntman.cpp
--- i/base/thirdparty/kpvcrlib/crengine/crengine/src/lvfntman.cpp
+++ w/base/thirdparty/kpvcrlib/crengine/crengine/src/lvfntman.cpp
@@ -728,7 +728,7 @@ public:
}
for (i = _instance_list.length(); i--; ) {
#ifndef NDEBUG
- LVFontRef &fnt = _registered_list[i]->_fnt;
+ LVFontRef &fnt = _instance_list[i]->_fnt;
assert(fnt.isNull() || fnt.getRefCount() == 1);
#endif
delete _instance_list.remove(i);
|
|
Your (logical, fixing a coding error that was affecting only debug builds) change to lvfntman.cpp solves all my segfault cases :) 👍 (I'm still on koreader/crengine@a32b9ed) (Didn't try the other changes, I guess you're just being super clean - but my idea with fork & die fast is that we just don't have any cleaning to waste doing.) |
|
^ Looks like I didn't really test much :/ |
|
Yes, after fixing the stupid mistake in |
|
No problem with adding #1696. But can you say why it does help with the segfaults (which were happening only in some cases with Harfbuzz or images around)? Also, is it because in ffi/ |
|
I don't think there's a way to prevent the finalizers from being called. The assert is triggered because there's actually still a document registered. #1696 is so |
It still bugs me to have to think what happens after we are done.
Why do we call it then ? :) Is that C code or Lua code ? May be it can be hard for C destructors to not be called, but it feels we should be able to suicide more easily in Lua :) |
|
Btw, this also prevents the segfaults: --- a/ffi/util.lua
+++ b/ffi/util.lua
@@ -376,6 +376,9 @@ function util.runInSubProcess(func, with_pipe, double_fork)
if not ok then
print("error in subprocess:", err)
end
+ print("killing -9 me")
+ C.kill(C.getpid(), 9)
+ print("killed -9 me")
os.exit(0)
end
-- parent/main processand it is in the spirit of this subprocess stuff, and what the parent process does when asked to terminate a subprocess: Lines 418 to 431 in 01efb98 So, if the parent does this awful thing to its child, I guess the child can be allowed to do it to itself :) Dunno if it should also be done in the intermediate process (which, I guess, should also call finalizers?) when double forking ? Lines 331 to 345 in 01efb98 Btw, @NiLuJe , what did you have in mind (Python?) when writing there: |
If there were he wouldn't have said it ought to exist I'd say, but simply used it instead. ^_^ |
|
I guess so :) but I asked what, when saying it, he had in mind but didn't say :) |
|
Relevant to our interests is that I'm also not sure if the glibc doesn't do something funky with that symbol, so it may not be easy to get at via FFI ;). |
|
I think all the destructors we may be talking about are not implicitely called on os.exit(). Lines 4123 to 4133 in 01efb98 So, if we don't want to go yet with a C.kill(C.getpid(), 9) (although, who knows what may happen in other libraries/engines like MuPDF/Djvu), the gentler solution would be to just prevent the above from happening when in a subprocess (ie. by setting a static cre.cpp variable, and exiting early from this cre_teardown() if set?
|
|
I really don't see the interest in that when those are essentially free? |
|
What that and those ? |
|
I mean trying to avoid destructors and sane cleanup (anywhere, anywhen, context is irrelevant ^^). |
|
I believe context is relevant... |
|
I fairly strongly disagree with that (on paper, at least), it seems to be a perfectly fun and interesting way to introduce weird and random issues due to the different behavior and potential codepaths taken.
On that front, you've already lost that fight by basically just having spawned a Lua fork at all ;). |
Bureaucrats, theoreticians... but we're talking about Life in the jungle here :)
The random issue and different behaviour we had above (really, we all 4 didn't manage to reproduce the same bug !) is because we took these same codepaths (that we didn't know/think/remember about, and didn't need to take)...
I don't think so. We just left the fighting cocks peaceful, avoiding any fight by disappearing early - with fork&exec for the dict lookup, by exiting as soon as done for the other stuff. I dunno if some Lua gc can happen during that, touching other objects - but it hadn't hit us all these years. Also dunno if this can more easily happen in a subprocess where we do network stuff (ie. Wikipedia lookup) and are indeed idle and waiting. |
This is one potential solution to fixing the regression introduced by koreader#1679. I don't support this solution, nor its friend of adding a `touch $(FRIBIDI_DIR)/include` at the bottom. Including (hah!) most these `/include`s seems to be a mistake. They are to be generated by their relevant targets. Either they fail or they don't, simple, no weirdness. They're targets for the target!
Partially reverts 8c133b0 (from koreader#1679). The logic behind including them doesn't check out. The targets in question depend on the files that are to be written in those very /include directories. If they fail to be created properly the target fails. Therefore including them in Makefile.third in this manner doesn't make much sense. I originally thought it was a bit weird but harmless, but as it turns out it's the exact opposite. So yeah, it has to go away ASAP.
|
So, how are we going about that issue? There is the proposed solution above at #1679 (comment) that solves it for this page thumbnail generations.
So, it still feels to me that a more generic suicide solution - or a "do not finalize" flag - is a lot better than having to plug many ad-hoc |
|
Zero feedback :( Adding the same kludge in these other run-in-subprocess function would be ugly.
My preference is still for the first option. |
|
I don't like it, no. I thought my position on the matter was clear: the mistake is using fork in the first place. And I believe disabling CRE finalizers so the asserts are not triggered is the equivalent of putting on a blindfold: just because you don't see it does not mean the problem is not still there. With that being said those forks are not going away any time soon, so… Pick a kludge: 1 is less work, 2 is cleaner (and there's already some others instances that could benefit from a way to do some pre-fork cleaning: see calls to |
I meant: disabling them in the forked process, after the fork: we know the subprocess is going to die soon, we don't care about any problem in it.
I mentionned "pre-fork" for completeness in a set of callbacks, but I don't see why we would need to do anything pre-fork - so, in the main process, that will continue to live on and whose state does not need to change. |
Take 2 of #1676: includes #1677, #1678, and use a custom
crsetup.hheader to avoid having to move the flags around.This change is