[webkit-reviews] review granted: [Bug 188144] REGRESSION (r230817): Terrible performance when selecting text on Stash code review : [Attachment 346024] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 188144: REGRESSION (r230817): Terrible performance when selecting text on
Stash code review
https://bugs.webkit.org/show_bug.cgi?id=188144

Attachment 346024: Patch

https://bugs.webkit.org/attachment.cgi?id=346024&action=review




--- Comment #15 from Darin Adler <darin at apple.com> ---
Comment on attachment 346024
  --> https://bugs.webkit.org/attachment.cgi?id=346024
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346024&action=review

> Source/WebKit/UIProcess/WebPageProxy.cpp:1953
> +    Vector<NativeWebMouseEvent, 1> eventsAfterReplacedEvent;

The algorithm here is unnecessarily inefficient. We can find the event to
replace by iterating the Deque without modifying the Deque. Then we can replace
it in place. There is no need to remove all the events after it and then
re-append them. This could be the function:

    auto it = queue.rbegin();
    auto end = queue.rend();
    // Must not merge with the first event in the deque, since it is already
being dispatched.
    if (it != end)
	--end;
    for (; it != end; ++it) {
	auto type = it->type();
	if (type == incomingEvent.type()) {
	    LOG(MouseHandling, "UIProcess: replacing mouse event %s (queue size
%zu)", webMouseEventTypeString(type), queue.size());
	    *it = incomingEvent;
	    return;
	}
	if (type != WebEvent::MouseMove && type != WebEvent::MouseForceChanged)
	    break;
    }
    LOG(MouseHandling, "UIProcess: enqueueing mouse event %s (queue size %zu)",
webMouseEventTypeString(incomingEvent.type()), queue.size());
    queue.append(incomingEvent);

I left the logging in, although I think it makes the code a little confusing.

> Source/WebKit/UIProcess/WebPageProxy.cpp:1966
> +	   auto lastEvent = queue.takeLast();
> +	   if (lastEvent.type() == incomingEvent.type()) {
> +	       performedReplacement = true;
> +	       break;
> +	   }
> +
> +	   eventsAfterReplacedEvent.append(WTFMove(lastEvent));

Two thoughts:

1) Could use lastEventType here instead of lastEvent.type().
2) Could write this a different way that I find clearer:

	if (lastEventType == incomingEvent.type()) {
	    queue.removeLast();
	    performedReplacement = true;
	    break;
	}

	eventsAfterReplacedEvent.append(queue.takeLast());

There’s no real reason to structure the code so that the takeLast is shared.

But I like the rewritten version above even more -- replacing in place seems
better.