Skip to content

Add batch event processing in lime/sdl#1319

Open
aprothman wants to merge 52 commits intoopenfl:developfrom
aprothman:BatchUpdate
Open

Add batch event processing in lime/sdl#1319
aprothman wants to merge 52 commits intoopenfl:developfrom
aprothman:BatchUpdate

Conversation

@aprothman
Copy link
Contributor

Provide the BatchUpdate(n) method in the SDLApplication class
that supports pulling multiple events at a time from the sdl
event queue. BatchUpdate does similar work to Update, but it
will never wait indefinitely for an event to become available,
and it accomplishes its work more efficiently. Events are
fetched to a buffer at the class level, and this buffer needs
to be freed in ~SDLApplication().

Because BatchUpdate will never wait indefinitely, it can be
called in Init to process all pending events at startup (they
can accumulate) and to initialize the framerate timer for targets
that make use of it. This allows us to clean up Update by
removing the firstTime boolean in its event loop.

Going forward, BatchUpdate is a tool that can be used to
refactor the event loop logic, improving it for all targets. It
can also be used to implement the event loop inversion of control
demonstrated at Haxe Summit 2019 for embedding OpenFL apps into
the windows of host applications.

aprothman added 30 commits May 15, 2019 12:28
Provide the BatchUpdate(n) method in the SDLApplication class
that supports pulling multiple events at a time from the sdl
event queue. BatchUpdate does similar work to Update, but it
will never wait indefinitely for an event to become available,
and it accomplishes its work more efficiently. Events are
fetched to a buffer at the class level, and this buffer needs
to be freed in ~SDLApplication().

Because BatchUpdate will never wait indefinitely, it can be
called in Init to process all pending events at startup (they
can accumulate) and to initialize the framerate timer for targets
that make use of it. This allows us to clean up Update by
removing the firstTime boolean in its event loop.

Going forward, BatchUpdate is a tool that can be used to
refactor the event loop logic, improving it for all targets. It
can also be used to implement the event loop inversion of control
demonstrated at Haxe Summit 2019 for embedding OpenFL apps into
the windows of host applications.
Caused BatchUpdate to return -1 when the event loop is inactive
and exiting. This allows BatchUpdate to communicate the same info
that Update does with its boolean return.
Manually resolved a merge conflict, keeping a previously added
line of code that was near an incoming change
Resolve conflict in SDLApplication.cpp, preserving BatchUpdate
in Init
Only allow garbage collection during the handling of a batch of
events when using the BatchUpdate program flow. Haxe garbage
collection is disabled between batches of events.
Since batch event processing means events are handled in a group
followed by a slightly longer wait before the next batch of events
is handled, there can be a performance impact for a mouse with a
fast update frequency. If more than one mouse motion event is
received in a batch, there's no need to handle all of them. By only
handling the most recently enqueued motion event, we're avoiding
unnecessary code running.
void ProcessTouchEvent (SDL_Event* event);
void ProcessWindowEvent (SDL_Event* event);
int WaitEvent (SDL_Event* event);
int GetPendingEvents (SDL_Event* event, int NumEvents);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the argument be int maxEvents to match the implementation?

queueLength = 0;
queueMaxLength = 0;
} else {
queueMaxLength = queueLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like queueMaxLength is the wrong name if you can just increase it. From the name, I'd expect excess events to be discarded or something.

Actually, what is the queueLength variable even for? It stores the last value of numEvents, but it's never needed outside this function. We could have queueLength do what queueMaxLength is currently doing, and use numEvents as the number of events. (That is, find and replace "queueLength" → "numEvents", then replace "queueMaxLength" → "queueLength".)


// handle the mouse motion event if there is one this batch
if (NULL != mouseMoved) {
HandleEvent (mouseMoved);
Copy link
Contributor

Choose a reason for hiding this comment

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

As I said elsewhere, I'm not a fan of doing this out of order. It could get skipped if the above loop returns. Another approach would be to add a second loop above the first:

for (nextEvent = 0; nextEvent < numPending; ++nextEvent) {
    if (eventQueue[nextEvent].type == SDL_MOUSEMOTION) {
        if (mouseMoved != NULL) mouseMoved->type = -1;
        mouseMoved = &eventQueue[nextEvent];
    }
}

This will set all but the final mouse moved event's type to -1. Then the main loop would skip over those events, and execute all >= 0 events in the right order.


int SDLApplication::BatchUpdate (int numEvents) {

SDL_Event* mouseMoved = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well move this down to where it's used.

int numPending = GetPendingEvents (eventQueue, queueLength);

// only allow GC free objects while we're handling events
if (isGCBlocking) System::GCExitBlocking ();
Copy link
Contributor

Choose a reason for hiding this comment

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

As I said elsewhere, this seems disruptive. Maybe it's appropriate, but the comment doesn't do a good job of explaining why.

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.

2 participants