Closed Bug 957659 Opened 6 years ago Closed 6 years ago

slim down WidgetKeyboardEvent::GetDOMKeyName

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: froydnj, Unassigned)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

This function is way too big for what it does (over 9K); let's make it significantly more efficient.
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?
Attachment #8357196 - Flags: review?(bugs)
Attachment #8357196 - Flags: review?(bugs) → review?(masayuki)
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.
Attachment #8357196 - Flags: review?(masayuki)
(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.
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?
Attachment #8358377 - Flags: review?(masayuki)
(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
Thank you for pointing out WidgetEventImpl.cpp.  I split this part out
for easier reviewing; I'll combine the two pieces on commit.
Attachment #8357196 - Attachment is obsolete: true
Attachment #8358382 - Flags: review?(masayuki)
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)
Attachment #8358377 - Flags: review?(masayuki) → review+
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?
Attachment #8358382 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/mozilla-central/rev/896367079635
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
Depends on: 962119
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.