Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tackle mcode_alloc issues once and for all #283

Merged
merged 100 commits into from
Jan 2, 2021

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Dec 31, 2020

Okay, ignore the insane commit history, because this went through a lot of experimentation, especially since I found out my old Nexus 5 on 7.1 didn't give a damn about the initial workaround that yielded decent results on Android 10...

Anyway.

  • Move to a shared luajit library. This gets rid of the weird hack where the launcher was named libluajit for... reasons (related, ironically, to static linking, and old broken API levels).
  • Simplified the dlopen replacement given that fact, and actually implemented the system libs blacklist properly (not that it should be necessary).
  • And, as far as the LuaJIT mcode_alloc issues go:
  • Make sure the launcher requests a single 512K block right at startup.
  • Patch LuaJIT so that:
    • Flushing all traces doesn't unmap the mcode area, but instead zero it, so we don't "lose" our hard-earned mmap, given how tighly packed the virtual memory layout is on Android.
    • Reserve a 1MB area inside LuaJIT's address space via a global non-const, initialized array. This pretty much guarantees it'll be in the jumprange ;).
    • Make sure the very first alloc will punch a hole in that array, via MAP_FIXED

This change is Reviewable

Which implies renaming our main entry point ;).
But it works.
It works.
It's a plain array, and order is *very much* important here...
when I'm using the same thing in C without issue...

Weird-ass symbol shenanigans with multiple dlopen implementations?
But we're back to the random ffi.load failures (unless the JIT is
disabled).

The already loaded check doesn't make much sense, either.
Copy link
Member

@pazos pazos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NiLuJe: lgtm

A minor nit is the dummy c file needed to bundle libluajit.so. That could be avoided if we copy libluajit.so to a given path (libs/$(ANDROID_ABI)) and tell gradle to pick external libraries from there.

I think everything related to luajit can be managed from mk-luajit.sh and we can get rid of it in Android.mk.

@NiLuJe
Copy link
Member Author

NiLuJe commented Jan 1, 2021

@pazos: This iteration got rid of that ;). (I mean, the source is still there because I left the behavior optional for documentation purposes, but it's commented out and ifdef'ed out ;)).

@NiLuJe
Copy link
Member Author

NiLuJe commented Jan 1, 2021

But, yes, gradle would probably be the place to handle that properly. I just don't know how the hell this thing works ^^.

@pazos
Copy link
Member

pazos commented Jan 1, 2021

But, yes, gradle would probably be the place to handle that properly. I just don't know how the hell this thing works ^^.

Please merge this PR and I can take care of it next. I would merge it myself but I'm not sure if you want to keep the commit history or squash all commits together

@NiLuJe
Copy link
Member Author

NiLuJe commented Jan 1, 2021

Oh, definitely squash ;p.

Just have to double-check that the last commit didn't break everything ;p.

Spoiler: It did ;p. I've since fixed it, though :).

@NiLuJe NiLuJe merged commit af9b9d3 into koreader:master Jan 2, 2021
NiLuJe added a commit to koreader/koreader that referenced this pull request Jan 2, 2021
* Don't flag Android as should_restrict_JIT

And allow disabling the C blitter, to put the workaround to the test...

* Add a -d, --debug flag to the log function

Catches KOReader's debug, as well as (our) dlopen & luajit logging

* Bump android-luajit-launcher

koreader/android-luajit-launcher#283
koreader/android-luajit-launcher#282

* Bump base

(koreader/koreader-base#1279)
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Sep 28, 2022
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Oct 2, 2022
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Oct 3, 2022
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Oct 5, 2022
NiLuJe added a commit to koreader/koreader that referenced this pull request Oct 6, 2022
Basically:

* Use `extend` for class definitions
* Use `new` for object instantiations

That includes some minor code cleanups along the way:

* Updated `Widget`'s docs to make the semantics clearer.
* Removed `should_restrict_JIT` (it's been dead code since koreader/android-luajit-launcher#283)
* Minor refactoring of LuaSettings/LuaData/LuaDefaults/DocSettings to behave (mostly, they are instantiated via `open` instead of `new`) like everything else and handle inheritance properly (i.e., DocSettings is now a proper LuaSettings subclass).
* Default to `WidgetContainer` instead of `InputContainer` for stuff that doesn't actually setup key/gesture events.
* Ditto for explicit `*Listener` only classes, make sure they're based on `EventListener` instead of something uselessly fancier.
* Unless absolutely necessary, do not store references in class objects, ever; only values. Instead, always store references in instances, to avoid both sneaky inheritance issues, and sneaky GC pinning of stale references.
  * ReaderUI: Fix one such issue with its `active_widgets` array, with critical implications, as it essentially pinned *all* of ReaderUI's modules, including their reference to the `Document` instance (i.e., that was a big-ass leak).
* Terminal: Make sure the shell is killed on plugin teardown.
* InputText: Fix Home/End/Del physical keys to behave sensibly.
* InputContainer/WidgetContainer: If necessary, compute self.dimen at paintTo time (previously, only InputContainers did, which might have had something to do with random widgets unconcerned about input using it as a baseclass instead of WidgetContainer...).
* OverlapGroup: Compute self.dimen at *init* time, because for some reason it needs to do that, but do it directly in OverlapGroup instead of going through a weird WidgetContainer method that it was the sole user of.
* ReaderCropping: Under no circumstances should a Document instance member (here, self.bbox) risk being `nil`ed!
* Kobo: Minor code cleanups.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants