55403 - PL_ProcessPendingEvents optimization

Status: RESOLVED INVALID

(Core :: XPCOM, defect, P3)

Description

[Doug Turner (:dougt)]

In an email from : Colin Blake <[e-mail]>


I was looking at the code in plevent, and came up with an idea for a speed-up.
We don't need to walk the queue while holding the lock to find out how many
events we have to process. We only need to find out which is the last event and
then we can release the lock and pluck events from the queue until we've
processed the last one. Any events which
have been added while we are processing we simply be ignored. Isn't this the
behaviour we want?

Modified BUT UNTESTED code is attached by ways of an example of what I mean.

FWIW...

Colin.


PR_IMPLEMENT(void)
PL_ProcessPendingEvents(PLEventQueue* self)
{
    PRInt32 count;
    PLEvent* last, event;

    if (self == NULL)
      return;

    PR_EnterMonitor(self->monitor);

    if (self->processingEvents) {
      PR_ExitMonitor(self->monitor);
      return;
    }
    self->processingEvents = PR_TRUE;

    /* Only process the events that are already in the queue, and
     * not any new events that get added. Do this by counting the
     * number of events currently in the queue
     */
    last = PR_LIST_TAIL(&self->queue);
    PR_ExitMonitor(self->monitor);

    if (last != &self->queue) { /* check queue was not empty */
      do {
        event = PL_GetEvent(self);
        if (event == NULL)
          break; /* should never get here */

        PR_LOG(event_lm, PR_LOG_DEBUG, ("$$$ processing event"));
        PL_HandleEvent(event);
        PR_LOG(event_lm, PR_LOG_DEBUG, ("$$$ done processing event"));
      }
      while (event != last);
    }
    rest as before...

Comment 1

[Brendan Eich [:brendan]]

I thought we did this, but...

PL_GetEvent has to grab the monitor each time.  Ok, at least you won't hold it
across dispatches.

What about RevokeEvents?  Could that happen in the middle of handling the seven
events we counted, invalidating our count?

/be

Comment 2

[Colin Blake]

RevokeEvents is bad news for both my suggestion and for the existing 
implementation. With my idea if someone revokes what was the last entry, then 
we'll end up processing ALL new events which get added to the queue. With the 
existing implementation, we only process SOME new events which get added. Either 
way, we need to think some more about revokation.

Comment 3

[Dan M]

2 

Comment 4

[Dan M]

This is related to bug 50104. Stabs I've taken at that bug make it seem like we 
could just keep a "count" variable in the event queue.

Comment 5

[Colin Blake]

I don't think a simple count will do. A count may become incorrect if you have 
to RevokeEvents while processing.  

Comment 6

[Dan M]

What would make it more complicated than adjusting the count in PL_PostEvent, 
PL_GetEvent and PL_DequeueEvent? And of course _pl_CreateEventQueue.

Comment 7

[Colin Blake]

Queue contains A, B, C when ProcessPending is called. You compute 3 as the
number of events to process.

Start processing A, count is now 2.

While processing A two more get queued, D and E. But you're smart and don't
touch the counter. Queue is now B, C, D, E. Your count is 2.

While still processing A, someone calls Revoke. How does revoke know whether
to decrement the count or not? The only way I see is to walk the queue to
locate the entry to revoke, and then, based upon its position, decide if the
counter should be decremented or not. This can be done, but involved locking
down the queue and walking it, which is what we were trying to avoid in the
first place.

Comment 8

[Dan M]

  True enough, but we don't require so much precision from the count in 
PL_ProcessingEvents. All we're using the counter for is to limit the time it 
spends in that function, so it doesn't stay there forever while new events come 
in as it processes extant ones. The exact count isn't important for our purposes.
  The current code just calculates a local copy of the count at the beginning, 
and stops processing after that many events have been processed, or when the 
queue is empty, whichever comes first. I'm suggesting we maintain an accurate 
live count, and keep the current code but just set the initial local count by the 
live count in the beginning, rather than stepping through the event queue; the 
thing you were wanting to improve here.
  It seems foolproof to me, and affected by revoking events no more than the 
current code.

Comment 9

[Colin Blake]

Now I see what you're getting at. Yes, you're right. It doesn't really matter
if revoke is called while we're processing, since the worst that'll happen is
that we'll process a few "extra" events. No big deal. Since all the queue
insertion/deletion is done under a lock, keeping a counter should be safe.
Sounds like a solution to me!

Comment 10

[Darin Fisher]

PL_ProcessPendingEvents no longer exists (see bug 326273). I think we can close this bug as INVALID now.