Input: Batched reads & batched table allocations #1346

NiLuJe · 2021-04-02T18:45:16Z

Okay, turns out, reading the full batch of events at once (or at least in as few read as possible) isn't too gnarly, and allows us to do nifty things with Lua table allocations, by telling Lua exactly how many elements we're going to insert.

This change is

A good night's sleep does wonder ;).

We're looping anyway, so the size isn't terribly important.

We know exactly how many fields we'll need.

No need for pointer shenanigans, read already returns a byte count

NiLuJe · 2021-04-02T18:45:28Z

(I'll punt the debug printfs after dinner ;)).

Not terribly useful logic-wise, unlike in waitForEvent, but at least it makes stuff consistent.

Re koreader/koreader#7485

Frenzie · 2021-04-02T20:46:25Z

input/input.c

+                    return 2;  // false, errno
+                }
+                if (len == 0) {
+                    // Should never happen


poire-z · 2021-04-02T21:04:26Z

ffi/posix_h.lua

-    pcall(ffi.load, "/lib/librt.so.1", true)
+    -- NOTE: There's no librt.so symlink, so, specify the SOVER, but not the full path,
+    --       in order to let the dynamic loader figure it out on its own.
+    pcall(ffi.load, "rt.so.1", true)


(Just letting you know this keeps working on my GloHD.)

Cool, thanks ;).

I'd checked with a dummy library and a strace that it was indeed following all the crazy stock paths, but glad to have a confirmation ;).

NiLuJe · 2021-04-02T21:25:25Z

Wheeee \o/

[pid 25222] 23:24:57.828218 [2ac7ae90] _newselect(8, [5</dev/input/event0> 6</dev/input/event1> 7<pipe:[72753]>], NULL, NULL, {tv_sec=1697, tv_usec=236071}) = 1 (in [6], left {tv_sec=1692, tv_usec=250573}) <4.985557>
[pid 25222] 23:25:02.814233 [2ac72e2c] read(6</dev/input/event1>, "\256\213g`\211k\f\0\3\09\0\1\0\0\0\256\213g`\217k\f\0\3\0000\0\1\0\0\0"..., 4096) = 112 <0.000036>
[pid 25222] 23:25:02.814570 [2ac72e2c] read(6</dev/input/event1>, 0x7ec627d8, 3984) = -1 EAGAIN (Resource temporarily unavailable) <0.000028>
[pid 25222] 23:25:02.815060 [2ac828dc] timerfd_create(CLOCK_REALTIME, TFD_CLOEXEC|TFD_NONBLOCK) = 11<anon_inode:[timerfd]> <0.000049>
[pid 25222] 23:25:02.815320 [2ac828fc] timerfd_settime(11<anon_inode:[timerfd]>, TFD_TIMER_ABSTIME, {it_interval={tv_sec=0, tv_nsec=0}, it_value={tv_sec=1617398703, tv_nsec=313977000}}, NULL) = 0 <0.000034>
[pid 25222] 23:25:02.816864 [2ac9031c] clock_gettime(CLOCK_MONOTONIC, {tv_sec=611936, tv_nsec=507595863}) = 0 <0.000034>
[pid 25222] 23:25:02.817232 [2ac7ae90] _newselect(12, [5</dev/input/event0> 6</dev/input/event1> 7<pipe:[72753]> 11<anon_inode:[timerfd]>], NULL, NULL, {tv_sec=1692, tv_usec=247057}) = 1 (in [6], left {tv_sec=1692, tv_usec=239888}) <0.007219>
[pid 25222] 23:25:02.824909 [2ac72e2c] read(6</dev/input/event1>, "\256\213g`\\\225\f\0\3\09\0\1\0\0\0\256\213g`b\225\f\0\3\0000\0\0\0\0\0"..., 4096) = 112 <0.000037>
[pid 25222] 23:25:02.825231 [2ac72e2c] read(6</dev/input/event1>, 0x7ec627d8, 3984) = -1 EAGAIN (Resource temporarily unavailable) <0.000026>
[pid 25222] 23:25:02.828375 [2ac9031c] clock_gettime(CLOCK_MONOTONIC, {tv_sec=611936, tv_nsec=519117922}) = 0 <0.000033>
[pid 25222] 23:25:02.828884 [2ac7ae90] _newselect(8, [5</dev/input/event0> 6</dev/input/event1> 7<pipe:[72753]>], NULL, NULL, {tv_sec=1692, tv_usec=235535}

NiLuJe

Reviewed 1 of 3 files at r1, 3 of 3 files at r3.
Dismissed @poire-z from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @NiLuJe)

ffi/posix_h.lua, line 165 at r2 (raw file):

Previously, NiLuJe (NiLuJe) wrote…

Cool, thanks ;).

I'd checked with a dummy library and a strace that it was indeed following all the crazy stock paths, but glad to have a confirmation ;).

Done.

koreader/koreader-base#1344 koreader/koreader-base#1345 koreader/koreader-base#1346

Requires koreader/koreader-base#1344 & koreader/koreader-base#1346 (fix #7485) Assorted input fixes: * Actually handle errors in the "there's a callback timer" input polling loop. * Don't break timerfd when the clock probe was inconclusive. Not directly related, but noticed because of duplicate onInputEvent handlers: * HookContainer: Fix deregistration to actually deregister properly. "Regression" extant since its inception in #2933 (!). * Made sure the three plugins (basically the trio of AutoThingies ;p) that were using HookContainer actually unschedule their task on teardown.

Okay, turns out, reading the full batch of events at once (or at least in as few `read` as possible) isn't too gnarly, and allows us to do nifty things with Lua table allocations, by telling Lua exactly how many elements we're going to insert. Also, fix loading librt on multilib systems.

NiLuJe added 22 commits April 2, 2021 16:29

Add EPIPE to the errno constants

b1dca22

Actually, reading the full queue isn't terribly complicated...

653625e

A good night's sleep does wonder ;).

Oops, don't reset the table index between read calls

d5d9402

Minor comment tweak

96191ba

That's a wee bit larger on 64-bit (64-bit time)

a982e69

Tone down the queue size

4352c52

We're looping anyway, so the size isn't terribly important.

Should match

5e4349e

Minor comment tweaks

2e04ed1

Squish debug

c4a0d6d

Start optimizing table allocations

2ede8c5

We know exactly how many fields we'll need.

And optimize the array of events allocations

50e5fba

Probably not worth obsessing over this ;p

55761d5

Buffered experiment...

d27452b

That was jumping too far ahead in the queue

6f0f3ab

Don't hardcode that

3161ef7

Mkay, that's not too terrible

541db55

Tweak comments

8177652

Avoid out pointers

0aa097c

Simplify queue size computations

89929ed

No need for pointer shenanigans, read already returns a byte count

More comment tweaks

e8980cd

More comment tweaks

3430cba

Ditto

ca2b17b

NiLuJe mentioned this pull request Apr 2, 2021

reader cannot start on cervantes after upgrade from v2021.03 to v2021.03-15 koreader/koreader#7485

Closed

NiLuJe added 2 commits April 2, 2021 22:25

Always pop function arguments from the stack when we're done with them

b41398d

Not terribly useful logic-wise, unlike in waitForEvent, but at least it makes stuff consistent.

Fix librt loading on multilib systems

137a343

Re koreader/koreader#7485

NiLuJe mentioned this pull request Apr 2, 2021

Input: Process input events in batches koreader/koreader#7483

Merged

Frenzie reviewed Apr 2, 2021

View reviewed changes

input/input.c

return 2; // false, errno

}

if (len == 0) {

// Should never happen

Copy link

Member

Frenzie Apr 2, 2021

Choose a reason for hiding this comment

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

lol

NiLuJe reacted with laugh emoji

poire-z reviewed Apr 2, 2021

View reviewed changes

Forgot a bit of comment ^^

1608306

NiLuJe added 2 commits April 2, 2021 23:20

Squish debug

5daaa98

clang-format pass

929bd2f

NiLuJe commented Apr 2, 2021

View reviewed changes

NiLuJe merged commit b3337e5 into koreader:master Apr 2, 2021

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Apr 2, 2021

Bump base

4d5eca3

koreader/koreader-base#1344 koreader/koreader-base#1345 koreader/koreader-base#1346

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Input: Batched reads & batched table allocations #1346

Input: Batched reads & batched table allocations #1346

NiLuJe commented Apr 2, 2021 •

edited by Frenzie

NiLuJe commented Apr 2, 2021

Frenzie Apr 2, 2021

poire-z Apr 2, 2021

NiLuJe Apr 2, 2021

NiLuJe commented Apr 2, 2021

NiLuJe left a comment

Input: Batched reads & batched table allocations #1346

Input: Batched reads & batched table allocations #1346

Conversation

NiLuJe commented Apr 2, 2021 • edited by Frenzie

NiLuJe commented Apr 2, 2021

Frenzie Apr 2, 2021

Choose a reason for hiding this comment

poire-z Apr 2, 2021

Choose a reason for hiding this comment

NiLuJe Apr 2, 2021

Choose a reason for hiding this comment

NiLuJe commented Apr 2, 2021

NiLuJe left a comment

Choose a reason for hiding this comment

NiLuJe commented Apr 2, 2021 •

edited by Frenzie