Bug 93123 - Meaning of deltaX/deltaY in PlatformGestureEvent is overloaded
Summary: Meaning of deltaX/deltaY in PlatformGestureEvent is overloaded
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rick Byers
URL:
Keywords:
Depends on: 94238 95573
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-03 07:47 PDT by Rick Byers
Modified: 2013-10-21 11:54 PDT (History)
7 users (show)

See Also:


Attachments
Patch (23.77 KB, patch)
2012-08-30 13:32 PDT, Rick Byers
no flags Details | Formatted Diff | Diff
Patch (22.16 KB, patch)
2012-08-31 06:55 PDT, Rick Byers
no flags Details | Formatted Diff | Diff
Patch (7.74 KB, patch)
2012-09-11 20:20 PDT, Rick Byers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rick Byers 2012-08-03 07:47:26 PDT
WebCore::PlatformGestureEvent has deltaX and deltaY fields that are used for a variety of different things for different gesture types:
  - For scroll update it's the scroll amount
  - For pinch it's a scale factor
  - For fling it's a velocity
  - For tap it's the tap count
This is confusing and error prone.

We should split these uses out into new fields, and where we care about wasting too much space new sub-classes of PlatformGestureEvent (or potentially a union).

I'm not sure what the best trade-off between simplicity and space is.  For example, PlatformMouseEvent keeps a tapCount field, even though it's only relevent for mousedown/mouseup/click events.  So to be consistent we should probably just add a tapCount to PlatformGestureEvent.

For the others (which really are x and y values), perhaps we should use "extraX" and "extraY" fields and just have properly-named accessors that validate they are only called when the event is of the required type?
Comment 1 Adam Barth 2012-08-03 10:07:06 PDT
I wouldn't worry too much about space.  There are only a handful of these objects at a time.  It's not like you're adding bytes to Node, of which there are thousands in each page.
Comment 2 Rick Byers 2012-08-16 11:39:39 PDT
There was some concern over the performance, especially given that we're trying hard to drive down latency of some gesture events (scroll updates in particular).  I just did some measurements, and I agree there's no point worrying about a few extra fields.

In particular (for those that don't want to take my word for it), here is what I did:
 - Added an array of N floats to chromium's WebGestureEvent, which all get initialized to 0
 - Added an identical array to PlatformGestureEvent, and copied each element one-by-one (in a for loop) from WebGestureEvent
 - Measured latency of about 100 tap events on ChromeOS with different values of N by plumbing high-resolution event timestamps through to javascript using this patch: https://codereview.appspot.com/6452060 and comparing event timestamps to window.performance.webkitNow().  
- I did this without any touch event handlers on the page so that we wouldn't have the extra overhead of touch event round trip (which could add ~10ms in my tests).

With no extra padding (N=0), average latency on my hardware was about 3.8ms (+/- 0.8ms)
With N=1024 or 10240, any difference in latency was indistinguishable from noise
For N=10k, the average latency went up to about 6ms.

So as long as we keep the number of additional fields under 1k, I don't think we should worry about performance at all.
Comment 3 Rick Byers 2012-08-30 13:32:26 PDT
Created attachment 161543 [details]
Patch
Comment 4 Build Bot 2012-08-30 13:56:22 PDT
Comment on attachment 161543 [details]
Patch

Attachment 161543 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13684827
Comment 5 Early Warning System Bot 2012-08-30 15:00:44 PDT
Comment on attachment 161543 [details]
Patch

Attachment 161543 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13683916
Comment 6 Robert Kroeger 2012-08-30 15:32:35 PDT
Have you considered using subclasses for the PlatformGestureEvent? While unions would seem to be the right choice for WebGestureEvent, it is not clear to me that PGEs could not benefit from division.

You could conceivably separate the WCIHi code from the PGE change if you want a smaller CL?
Comment 7 Rick Byers 2012-08-31 06:55:31 PDT
Created attachment 161690 [details]
Patch
Comment 8 Rick Byers 2012-08-31 07:35:45 PDT
(In reply to comment #6)
> Have you considered using subclasses for the PlatformGestureEvent? While unions would seem to be the right choice for WebGestureEvent, it is not clear to me that PGEs could not benefit from division.

Yeah, it's certainly an option.  But at first glance it doesn't seem worth the extra complexity to me (more types, more Builder types, etc.).  I think you could make the argument that almost every Platform*Event type could be split into a lot more sub classes (eg. PlatformMouseEvent has some fields that are only used by some event types).  What's the right trade-off?

jamesr@ what's your opinion?
 
> You could conceivably separate the WCIHi code from the PGE change if you want a smaller CL?

Sure.  I thought this was simple enough that it wasn't worth adding another serializing step (it's already 6 CLs back and forth between chromium and WebKit to do the full clean-up).  But if you think this is too big, I'm happy to split this piece into two.
Comment 9 Rick Byers 2012-08-31 09:48:32 PDT
Ok, I've split the WebGestureEvents changes off into bug 95573.  I'll upload a new patch here based against it.
Comment 10 Rick Byers 2012-09-11 20:20:34 PDT
Created attachment 163506 [details]
Patch
Comment 11 Ryosuke Niwa 2012-09-17 13:44:42 PDT
Comment on attachment 163506 [details]
Patch

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

> Source/WebCore/platform/PlatformGestureEvent.h:47
> +        memset(&m_data, 0, sizeof(m_data));

Why do we need to memset here? The rationale of this change should be explained in the change log.
Comment 12 Rick Byers 2012-09-17 13:54:54 PDT
Thanks for your comments!

(In reply to comment #11)
> (From update of attachment 163506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163506&action=review
> 
> > Source/WebCore/platform/PlatformGestureEvent.h:47
> > +        memset(&m_data, 0, sizeof(m_data));
> 
> Why do we need to memset here? The rationale of this change should be explained in the change log.

jamesr@ argued that we should use a union here to avoid wasting space for event-type specific data (like the tap-count I added for GestureTapDown).  If we're going to use a union, then memset to zero all the union fields on initialization seems like a good idea to me, but we could explicitly initialize all the union members or some subset of them (but which subset?).

Once we have consensus on the approach, I'll update the Changelog with more rationale.
Comment 13 Ryosuke Niwa 2012-09-17 14:04:52 PDT
I don't think we should memset m_data unless they're used, in which case, we should be using the initialization list anyways. Calling memset is quite expensive.
Comment 14 James Robinson 2012-09-17 14:12:12 PDT
I think memset() is the right thing to do for a union type.  I'm not sure what you mean about "quite expensive" - do you expect it to be more expensive than zeroing out the fields by hand?

Saying "use the initializer list" doesn't make a ton of sense for unions.
Comment 15 Ryosuke Niwa 2012-09-17 14:27:01 PDT
(In reply to comment #14)
> I think memset() is the right thing to do for a union type.

If we're going to use values in the union, then sure. But if we're not going to access/use the union, then there's no need to initialize it.

> I'm not sure what you mean about "quite expensive" - do you expect it to be more expensive than zeroing out the fields by hand?

Yes. memset results in a function call in many platforms.

> Saying "use the initializer list" doesn't make a ton of sense for unions.

Why not? Don't we know the type, i.e. which value in the union to use, at the time of initialization?
Comment 16 Hugo Parente Lima 2013-10-21 11:54:54 PDT
PlatformGestureEvent was removed from WebKit on bug 122650.