957659 - slim down WidgetKeyboardEvent::GetDOMKeyName

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nathan Froyd [:froydnj]]

This function is way too big for what it does (over 9K); let's make it significantly more efficient.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: slim down WidgetKeyboardEvent::GetDOMKeyName

Instead of calling nsAString::Assign several hundred times along with a
switch statement that may or may not get translated into a jump table,
let's implement our own lookup table that we can guarantee will be
constant time.  The KeyNameTable struct is used instead of the more
obvious |const char* const table[]| idiom to avoid runtime relocations.

This saves ~10K or so on ARM Android.

One drawback to this approach is that if non-ASCII keynames get added,
things will...not go well.  I guess we can rely on tests for those
keynames to catch such cases?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8357196 [details] [diff] [review]
slim down WidgetKeyboardEvent::GetDOMKeyName

>+    // We do this complicated dance to make this function low overhead,
>+    // both in speed and space.  See bug 957659 for details.

I think that you should explain all of them instead of point the bug#.

>+#define KEY_STR_NUM_1(line) key##line
>+#define KEY_STR_NUM(line) KEY_STR_NUM_1(line)

Why do we need to define KEY_STR_NUM_1?

>+    struct KeyNameTable {

"{" should be next line (bottom of "s").

> #define NS_DEFINE_KEYNAME(aCPPName, aDOMKeyName) \
>-      case KEY_NAME_INDEX_##aCPPName: \
>-        aKeyName.Assign(NS_LITERAL_STRING(aDOMKeyName)); return;
>-    switch (aKeyNameIndex) {
>+      char KEY_STR_NUM(__LINE__)[sizeof(aDOMKeyName)];
> #include "nsDOMKeyNameList.h"
>-      case KEY_NAME_INDEX_USE_STRING:
>-      default:
>-        aKeyName.Truncate();
>-        return;
>+#undef NS_DEFINE_KEYNAME
>+    };
>+
>+    static const KeyNameTable keyNameTable = {

Rename it to kKeyNameTable.
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Prefixes

>+#define NS_DEFINE_KEYNAME(aCPPName, aDOMKeyName) aDOMKeyName,
>+#include "nsDOMKeyNameList.h"
>+#undef NS_DEFINE_KEYNAME
>+    };

Did you confirm this doesn't cause bustage on some environment? I worry about the "," after the last item.

>+
>+    static const uint16_t keyNameOffsets[] = {

kKeyNameOffsets.

>+#define NS_DEFINE_KEYNAME(aCPPName, aDOMKeyName)  \
>+      offsetof(struct KeyNameTable, KEY_STR_NUM(__LINE__)),
>+#include "nsDOMKeyNameList.h"
>+#undef NS_DEFINE_KEYNAME
>+    };
>+
>+    static_assert(KEY_NAME_INDEX_USE_STRING == ArrayLength(keyNameOffsets),
>+                  "Invalid enumeration values!");
>+
>+    if (aKeyNameIndex >= KEY_NAME_INDEX_USE_STRING) {
>+      aKeyName.Truncate();
>+      return;
>     }
>-#undef NS_DEFINE_KEYNAME
>+
>+    uint16_t offset = keyNameOffsets[aKeyNameIndex];
>+    const char* table = reinterpret_cast<const char*>(&keyNameTable);
>+
>+    aKeyName.AssignASCII(table + offset);
Looks like this needs to convert from char* to char16_t* at runtime. Why don't you use char16_t and MOZ_UTF16() for KeyNameTable? For footprint? Although, non-ASCII characters won't be used for predefined key names.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> >+#define KEY_STR_NUM_1(line) key##line
> >+#define KEY_STR_NUM(line) KEY_STR_NUM_1(line)
> 
> Why do we need to define KEY_STR_NUM_1?

This is required because of the way ## concatenation is defined to work: the literal value of |line|, rather than the expanded value of |line| is used.  If we just had:

#define KEY_STR_NUM(line) key##line

then KEY_STR_NUM(__LINE__) would be expanded to key__LINE__, which is not what we want.  So we need this two-level structure to force __LINE__ to be evaluated before we use it for concatenation.

> >+#define NS_DEFINE_KEYNAME(aCPPName, aDOMKeyName) aDOMKeyName,
> >+#include "nsDOMKeyNameList.h"
> >+#undef NS_DEFINE_KEYNAME
> >+    };
> 
> Did you confirm this doesn't cause bustage on some environment? I worry
> about the "," after the last item.

We have a lot of cases where we use trailing commas in similar contexts, so I am not worried here.

> >+    uint16_t offset = keyNameOffsets[aKeyNameIndex];
> >+    const char* table = reinterpret_cast<const char*>(&keyNameTable);
> >+
> >+    aKeyName.AssignASCII(table + offset);
> Looks like this needs to convert from char* to char16_t* at runtime. Why
> don't you use char16_t and MOZ_UTF16() for KeyNameTable? For footprint?
> Although, non-ASCII characters won't be used for predefined key names.

I did it for footprint, mostly.  My revised patch uses char16_t.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: slim down WidgetKeyboardEvent::GetDOMKeyName, v2

Updated with review comments addressed and extra checking that we
definitely have ASCII key names.

This method is getting awfully big (in terms of source code lines); is
there a good place to put it out of line instead of in this header file?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Nathan Froyd (:froydnj) from comment #4)
> This method is getting awfully big (in terms of source code lines); is
> there a good place to put it out of line instead of in this header file?

http://mxr.mozilla.org/mozilla-central/source/widget/shared/WidgetEventImpl.cpp

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: move GetDOMKeyName to WidgetEventImpl.cpp (to be folded)

Thank you for pointing out WidgetEventImpl.cpp.  I split this part out
for easier reviewing; I'll combine the two pieces on commit.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8358377 [details] [diff] [review]
slim down WidgetKeyboardEvent::GetDOMKeyName, v2

>+#define KEY_STR_NUM_1(line) key##line

nit: I like KEY_STR_NUM_INTERNAL(line) better.

>+#define KEY_STR_NUM(line) KEY_STR_NUM_1(line)

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8358382 [details] [diff] [review]
move GetDOMKeyName to WidgetEventImpl.cpp (to be folded)

Could you add

/******************************************************************************
 * mozilla::WidgetKeyboardEvent (TextEvents.h)
 ******************************************************************************/

above the new method?

Comment 9

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/896367079635

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/896367079635

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Nathan, did you check the .key value before landing? There is a report of broken key value. See bug 680830 comment 25.

If I have a chance to check if it's a regression of this, I'll do it. If you can check it before that, please do it and if so, please file a bug or backout the patch.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11)
> If I have a chance to check if it's a regression of this, I'll do it. If you
> can check it before that, please do it and if so, please file a bug or
> backout the patch.

It is a regression.  I have a patch, and will file a bug.