Repository navigation
Towards consistent stream polling API across baremetal/unix ports #1550
Description
Activity
One glaring incompatibility is return value of poll.poll() - instead of list of tuples (fd, flags) it returns just list of "fds".
? https://github.com/micropython/micropython/blob/master/stmhal/moduselect.c#L250-L251
http://docs.micropython.org/en/latest/library/select.html#select.poll.poll
Wait for at least one of the registered objects to become ready. Returns list of ready objects, or empty list on timeout.
Nowhere it makes clear that it returns list of tuples. I'll take care of updating the docs. Thanks.
Ok, docs are updated: 98b6d35 .
Next issue: stmhal doesn't define symbolic constants for events. And then Linux e.g. has: http://lxr.free-electrons.com/source/include/uapi/asm-generic/poll.h#L5 . So, there's POLLPRI stuck in front of POLLOUT, making it have value of 4. At least, as that file says, values are governed by iBCS2, which is still "intel", but it being in asm-generic gives out good hope...
Anyway, what to do about this?
Anyway, what to do about this?
I'd say add
POLL*constants to stmhal's select module, called as per CPython (POLLIN, POLLOUT, etc). And to keep things sane and portable, lets just use the numeric values as per asm-generic/poll.h (then same will go for errno, and others).Thanks, that's the solution I'd be looking for. Any concerns about handling of integer 2 value? We could check for it, and throw an error with instructions on upgrade. If there're no big concerns, I'd rather skip that, as it's carrying around unneeded luggage.
@pfalcon regarding 98b6d35 : returning a tuple which is not exactly 2 elements in size might lead to compatibility issues with CPython. Eg code that does
obj, ev = result[0]will be broken. What do you think?Any concerns about handling of integer 2 value?
I don't have big concerns, so we can skip providing an upgrade warning. What will happen instead is that polled objects will just never be available for "reading" so that should be a relatively easy error to track down.
Eg code that does obj, ev = result[0] will be broken. What do you think?
So, the idea behind that stuff is to be able to specify "callback data" in Poll.register() and get that data back in .poll() result. This is required for efficient processing of poll results. Yes, that's extension to CPython, but will let to standardize on simple clean interface Poll provides. The way CPython does it is via "selectors" module. I was ready to implement it for unix port (in micropython-lib), but I think it will be too much bloat for bare-metal. So, given all these reasons, I think it's acceptable if "obj, ev = result[0]" won't work and more specific code is required.
Note that all this is tentative yet and will take time to be fully worked out, and there's possibility to handle the case discussed in compatible way (by returning 2-tuple in case non-standard callback data wasn't passed). But I'm actually thinking about optimizations which will put even more constraints on what can be done with result of .poll() - like mentioned above having a singleton return structure and refilling it on each return, instead of allocating new each time.
So, the idea behind that stuff is to be able to specify "callback data" in Poll.register() and get that data back in .poll() result. This is required for efficient processing of poll results.
I don't have any experience using selectors so would need to see and understand a use case of returning "callback data" before I could form a better opinion on this. I'd really rather not break CPython compatibility like this if we really don't need to. (Although one could look through CPython lib to see if this would indeed break things. It might not.)
having a singleton return structure and refilling it on each return, instead of allocating new each time.
Could always implement
poll.poll_into(list[, timeout])I don't have any experience using selectors so would need to see and understand a use case of returning "callback data" before I could form a better opinion on this.
That's one of pretty big advantages of Linux epoll vs standard POSIX poll - almost always you need more state to handle particular connection than just fd. epoll allows to just set arbitrary data structure, thread pointer, coroutine pointer, and get it back directly (and know all this stuff is handled efficiently, in kernel). With poll, you need to map returned fd into your data/control structure yourself.
This feature is used by uasyncio for example - it associates a coroutine with waited fd, to resume it directly.
having a singleton return structure and refilling it on each return, instead of allocating new each time.
Could always implement poll.poll_into(list[, timeout])Yes, but why? The implementation I described would still be compliant with description of .poll() - indeed, it doesn't prohibit such behavior.
This feature is used by uasyncio for example - it associates a coroutine with waited fd, to resume it directly.
And well, to pronounce it explicitly (though I guessed it was obvious), the drive of this work is to run uasyncio on baremetal ports.
Ok, with bunch of thinking, and practical hurdles like #583 (comment) , I have following plan for unix implementation of Poll:
- Move it into C code.
- Base it on POSIX poll() after all. The motivation for using epoll() was the highest promised performance. But:
2.1. I read suggestion that for less than 1000 monitored fds, there won't be noticeable performance difference in poll() vs epoll(). Also, sources suggest that benefit of epoll() depends of type of workload (because it takes a syscall to add/remove each individual fd to a polled set, so if it's done too often, syscall overhead can eat into performance).
2.2. Poll is more portable after all.
2.3. As described above, epoll allows to map arbitrary object to fd, but due to way uPy GC works, we still need to keep collection of such mapped objects, to preclude them from GCing (they are referenced by kernel, but uPy doesn't know that, so if other Python data doesn't keep ref to it, it may be collected). Then, this bookkeeping collection is either memory-hungry, or not so efficient. So, epoll + bookkeep may have about the same performance as poll + simpler bookkeep. - I also would like to have mechanism to poll for non-fd objects (e.g. lwIP sockets). This also calls for C impl.
Yes, by why? Such implementation would still be compliant with description of .poll() - indeed, it doesn't prohibit such behavior.
True, it does return a list. But the tuples in the list would need to be reallocated each time because a tuple is read only you can expect to keep a reference to it and for it not to change.
And if there is ever multithreading support in uPy, it'd need a separate poll buffer for each thread.
Regarding epoll: it seems that the Python interface to this does not support registering additional data along with the fd.
I also would like to have mechanism to poll for non-fd objects (e.g. lwIP sockets). This also calls for C impl.
This is how stmhal (and cc3200?) work, since there is not really any concept of a file descriptor.
If you can use non-fd objects then that provides a way to attach data to the polling event: just pass a tuple
(fd, data)as the entity to register, and then the same tuple is returned when an event occurs. That way the GC refs are automatically stored.because a tuple is read only you can expect to keep a reference to it and for it not to change.
Keeping a reference to tuple returned from poll.poll() doesn't make sense.
And if there is ever multithreading support in uPy, it'd need a separate poll buffer for each thread.
Well, all buffers belong to particular poll object instance.
Regarding epoll: it seems that the Python interface to this does not support registering additional data along with the fd.
But C does, and micropython-lib supports that, and usasyncio depends on it.
This is how stmhal (and cc3200?) work, since there is not really any concept of a file descriptor.
Sure, and I'm talking about integrating of both native POSIX fd's and user-space i/o objects.
If you can use non-fd objects then that provides a way to attach data to the polling event: just pass a tuple
Well, tuple definitely doesn't know how to be waited on. And even tuple is pretty expensive object - I'm currently doing optimizations on uasyncio to reduce memory allocation for bookkeeping purposes like argument storage. (You should like this approach of avoiding memory allocation, right? ;-) )
Other thoughts of optimizing poll implementation (with uasyncio as client, as usual):
- Poll constructor should take an initial capacity.
- Also, for robustness, should take maximum capacity.
- Need to have operation "register or modify". Not sure how to go about this - add separate method like .regmod() or a flag to .register() or .modify() ?
- Need to support one-shot mode (after event fired, it (or all events??) are automatically disabled for fd). This is again how uasyncio works - as it emulates sequential program flow, it's interested only of next event of particular, and once it fired, not interested in anything else on that fd unless explicitly asked for something again (or otherwise a coroutine in .sleep() may be woken up up by data arriving on fd it read some time in the past - but heck, now there's nothing can even read it - coroutine code does something else). epoll allows to register one-shot mode per-fd, by ORing with EPOLLONESHOT. There's no place to store such flag for poll() to emulate the behavior, only if trying to use "unused" POLL* bits. But that's gotta be portability hazard. But as described above, such granularity isn't needed for uasyncio. So, I propose to add flag to poll.poll() to make it work in one-shot mode.
I also consider an optimization, where poll has special knowledge about socket objects, and each socket object stores its last used index in a poll map (which is just an array for POSIX poll()).
10 remaining items
Make unix poll.poll() return file object if that was passed in, making it consistent with baremetal poll (at the expense of storing these file objects in poll object).
Implemented in 093a8f5.
File-like objects then need to be hashable, so can be used as keys in a dict.
Before I go and patch all files and sockets to implement MP_UNARY_OP_HASH, please confirm that you don't want to got for solution 4. It would be much better solution. For extmod/moduselect, We can stop caching ->ioctl and use it for user data, while staying within 16 bytes. One caveat is that it's useful to have 2 words of user data, e.g. a callbacks and arguments (single one or tuple of many). But we can scope-creep it step by step ;-).
I wonder if you're ready for p.4, after a year in discussion.
Point 4 is good (it's efficient) but the question for me is about the user-facing API. I don't want to have poll.poll() return a 3-tuple and breaking compatibility with CPython.
The "selectors" module is really what we need here so why not just implement it? It can be done efficiently (in C) using pretty much all the existing code in extmod/moduselect. Just make existing poll_register and poll_modify take an extra optional "data" arg, and set up some aliases DefaultSelector=PollSelector=select.poll.
If that's too much, then just do the modifications to add "data" args to register and modify, and have poll.poll() return a 2-tuple but where the first arg is a tuple of the file-like-object and the user data.
Point 4 is good (it's efficient) but the question for me is about the user-facing API. I don't want to have poll.poll() return a 3-tuple and breaking compatibility with CPython.
My question is mostly about desire to store additional information in poll objects. API-wise, we can do .poll_into(), or maybe even .poll_iter() which would return an iterator.
The "selectors" module is really what we need here so why not just implement it?
That was my original idea, and how it evolves after that is I'm trying to add selectors-like functionality to existing select, to not bloat with adding a separate module (especially that for efficiency, we need another "into"-style API anyway).
Ok, I'd like to move further with this. The key point is whether we can constrain number of returned events for different underlying polling implementations to 1. Answer: we can for baremetal poll (we can do anything), we can for Linux epoll (epoll_wait takes maxevents param). But we can't for standard POSIX poll().
So, then:
Have an poll.ipoll() return an iterator (allocate-once apparently). Then its
__next__()is to return once-allocated list (given users a hint that they shouldn't store it) of [fileobj, ev, userdata] (maybe more elements later).So, the use would be:
for f, ev, data in poller.ipoll(delay): ...Btw, using internal pre-allocated list and sequence unpacking may make a more natural Python syntactic pattern after all, then external pre-allocated list, passed as an argument. Not as explicit about the fact that the list may change between the invocations, though.
Have an poll.ipoll() return an iterator
Sounds ok, hopefully it indeed doesn't allocate RAM in practice.
So, the use would be:
for f, ev, data in poller.ipoll(delay):If code is going to be written like that then we can't change the number of elements returned by ipoll's next. So should make a decision now that
[fileobj, ev, userdata]is good enough.If code is going to be written like that then we can't change the number of elements returned by ipoll's next.
How we can't, of course we can. I don't see any issue even with adding extra param to .poll() - we have bigger differences anyway. And of course we can state what's implied explicitly in the docs - "this function is provisional for next 5 (or 10) years". But the caveat is that there's no docs for .ipoll(), and before they can be written, docs for other things having existed for some time, need to be written first.
Btw, with the plan like above I keep thinking whether returning a "caller-owned" tuple is better after all (than a list). With a tuple there's more or less big issue - that it would be changing tuple, but at least it's the only one. And it would be hard to abuse it for bad. So, we just document it as a special kind of tuple, voila, problem solved. With a list, there're lot of "smaller", but more severe issues, like users being able to change size of this list, etc. So, a reliable solution will be inventing a special type of list whose size can't be changed and some other operations are disabled. Well, it's better to invent a special kind of tuple, whose values can change by external force. At least that requires 0 lines of code.
How we can't, of course we can.
But then we break our own code as well as other people's... not a fun situation. The other way of doing it is
for (f, ev, data, *rest) in poller.ipoll(delay):That allows for changes in ipoll's return tuple. Except it probably allocates on the heap. So then just a simplefor x in poller.ipoll(delay): x[0], x[1], x[2]etc.Btw, with the plan like above I keep thinking whether returning a "caller-owned" tuple is better after all (than a list).
I think at this stage the best thing to do is make a proof of concept patch based on the discussion so far, then evaluate the code.
I'm curious about this:
File-like objects then need to be hashable, so can be used as keys in a dict.
Has there been any progress on this front? I naively tried to set up a dictionary of
<object returned by poll>: <handler function>, which of course didn't work. I'm usingid(<object returned by poll>)right now, which works but seems hacky.I'm using id(<object returned by poll>) right now, which works but seems hacky.
Per https://github.com/micropython/micropython/wiki/ContributorGuidelines#django-criteria , that's the correct approach. Anything you can do, please do the way you can. The focus of adding new features is features which can't, or very cumbersome, to do otherwise.
Careful inspection shows that we still have consistency issues:
-
Unix poll.modify(obj, events) doesn't raise ENOENT if obj doesn't exist, while baremetal does. Given that there's a way to avoid raising exception - use .register() (which will just add object if not yet in poller), then the fix is clear - unix should raise exception too.
-
The situation with poll.unregister(obj) is more subtle - both modules don't raise ENOENT if obj is not in poller, while CPython docs say they should. The reason for this is that not raising exceptions, i.e. idempotent operation, is "more optimal". Funnily, uasyncio still has try/except bracket for it, apparently from times when uasyncio still ran under CPython. At the same time, discussion at uasyncio: KeyError in
remove_reader(MacOS) micropython-lib#248 shows that people (cluelessly) rely on no-raise behavior.
So, we need to decide what to do about unregister(). Either we:
- Say that specifically for the
uselectmodule, the semantics of unregister() is that it doesn't raise an exception, or - We add an optional flag to either raise exc in CPython compatible way, or skip raising exception (in this case, probably worth returning a bool telling whether obj was found or not).
It's worth thinking how to do that flag right, as it's probably not the last case when it's useful to select between raising exceptions and not. I believe we already had a discussion that usage of not negated words is preferred, e.g.
raise=Falseis preferred tono_raise=True. Butraiseis keyword anyway, so we'd usethrow, which would be similar terminology as for generator's .throw() method.And still, "nothrow" is a term on its own, e.g. https://en.cppreference.com/w/cpp/memory/new/nothrow (damn, they now have noexcept too: https://en.cppreference.com/w/cpp/language/noexcept_spec).
But if this flag will ever be a keyword param (not expected to happen), just be able to reuse "throw" qstr calls for it to be named "throw", not "nothrow".
-
- Unix poll.modify(obj, events) doesn't raise ENOENT if obj doesn't exist,
This was fixed in b9bad7f
2. The situation with poll.unregister(obj) is more subtle - both modules don't raise ENOENT if obj is not in poller, while CPython docs say they should.
This is tracked by a TODO in the code.
I'd say this issue can be closed. The
selectmodule has been working well for a while now, withuasynciov3 running well on both unix and bare-metal. #6125 discusses future optimisations of stream polling.
What we have now is:
Proposed: