Closed Bug 1583766 Opened 5 years ago Closed 5 years ago

3.26 - 3.52% tart (windows10-64-shippable, windows7-32-shippable) regression on push eff2ef054e11b67054afd0efad550bce37a13198 (Fri September 20 2019)

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 --- fixed
firefox71 --- verified

People

(Reporter: alexandrui, Assigned: masayuki)

References

(Regression)

Details

(4 keywords)

Attachments

(1 file)

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=eff2ef054e11b67054afd0efad550bce37a13198

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

4% tart windows10-64-shippable opt e10s stylo 2.64 -> 2.73
3% tart windows7-32-shippable opt e10s stylo 2.63 -> 2.72
3% tart windows7-32-shippable opt e10s stylo 2.63 -> 2.71

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=23217

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/TestEngineering/Performance/Talos

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/TestEngineering/Performance/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/TestEngineering/Performance/Talos/RegressionBugsHandling

Flags: needinfo?(masayuki)
Component: Performance → User events and focus handling
Product: Testing → Core
Version: Version 3 → unspecified

Ah, I realize that we can skip the cost when observer->mNeedsToNotifyIMEOfTextChange was not set to true. I'll post a patch on next Monday (I'll be back from the vacation).

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
Priority: -- → P2

The previous change is wrong because the sender shouldn't send selection change
notification if IMEContentObserver does not receive text change notifications
nor selection change notifications.

This patch reverts the change in the if block of previous change and instead
sets IMEContentObserver::mNeedsToNotifyIMEOfSelectionChange to true if
the sender sends a text change notification. (Note that even if there is
another text change with a mutation event listener, we need to send selection
change notification later so that this must be the right approach.)

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/51f34b686dac
Consider `IMEContentObserver` to send selection change notification when its sender sends a text change notification r=m_kato
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

== Change summary for alert #23299 (as of Tue, 01 Oct 2019 10:33:07 GMT) ==

Improvements:

3% tart windows10-64-shippable opt e10s stylo 2.74 -> 2.67
3% tart windows10-64-shippable opt e10s stylo 2.74 -> 2.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=23299

Comment on attachment 9097258 [details]
Bug 1583766 - Consider IMEContentObserver to send selection change notification when its sender sends a text change notification

Beta/Release Uplift Approval Request

  • User impact if declined: The previous uplifted patch caused performance regression.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: 1. Open Word, Excel and PowerPoint of Office Online and start editing a file
  1. Press "Comments" button in the toolbar (at right)
  2. Press "New" to show a comment field.
  3. Type something in the first line
  4. Type Enter some times (to make empty lines)
  5. Type a character with IME at last line
  6. Type Backspace key some times to remove the last line and empty lines (don't remove all empty lines at this step, i.e., you should stay in an empty line after this step)
  7. Type something with IME and convert it with IME (Type "a" -> "i" and Type space key if you use Japanese IME)

Then, at step 8, suggest window (shown at typing characters) and candidate window (shown at typing space character) should be shown immediately under caret.

  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The previous patch made it recompute selection range even if text change notification was not sent. That means that the previous patch made it recompute selection range all window position change and scroll. That's the reason of this regression. This patch reverts the previous patch and kicking the recomputation when it sends text change notification.
  • String changes made/needed: none
Attachment #9097258 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Hi, This issue is Verified as Fixed in 71.0a1 (2019-10-02) on Mac 10.14 and Windows 10 but I was unable to get the suggestions bar on Ubuntu, I was however able to add the text in Japanese on the second row where in older versions of Firefox I wasn't able to add any text after hitting backspace a few rows.

On Windows 7 after typing a character the Suggestions bar is not displayed only the candidate window is displayed after hitting space a few times but at least you can input a character as in previous versions you wouldn't be able to type anything after hitting backspace a few rows.

Masayuki can you please take a look and let me know if this is the intended behavior and if I can mark it Verified as Fixed ?

Flags: needinfo?(masayuki)

(In reply to Rares Doghi from comment #8)

Hi, This issue is Verified as Fixed in 71.0a1 (2019-10-02) on Mac 10.14 and Windows 10

Thank you!

but I was unable to get the suggestions bar on Ubuntu, I was however able to add the text in Japanese on the second row where in older versions of Firefox I wasn't able to add any text after hitting backspace a few rows.

Sorry, I couldn't parse the last sentence. Did you mean that you couldn't type text at the step8 with older Firefox, you can do it with Nightly? If so, the original bug affects something to the IME on Ubuntu (Mozc?). And if so, it's no problem.

It depends on IME whether the suggestion window is shown in that case or not. You can confirm the ideal behavior with simple <textarea> element like this "add comment" form of bugzilla with same operation. This bug fix and the previous bug fix try to make IME behave consistently between <textarea> and comment form of Office Online. (Anyway, I'm building Nightly on Ubuntu to check it with Mozc.)

On Windows 7 after typing a character the Suggestions bar is not displayed only the candidate window is displayed after hitting space a few times but at least you can input a character as in previous versions you wouldn't be able to type anything after hitting backspace a few rows.

It's correct behavior. IIRC, MS-IME introduced suggestion feature starting from Win8 (The feature is following third-party's IME behavior). So, not showing suggest window is right behavior. And the number of hitting space key to show candidate window depends on the settings. IIRC, its default value was 2 or 3.

Masayuki can you please take a look and let me know if this is the intended behavior and if I can mark it Verified as Fixed ?

I think that you verified the fix completely, even though the Ubuntu's behavior is unclear. But I fixed XP part, not platform specific part so that even if we still have a bug on Linux, it should be handled in another bug.

Flags: needinfo?(masayuki)

I don't think its an actual bug on Ubuntu, I really think its a system language issue more than anything with Firefox, as for the behavior at step 8 after going back a few spaces in older versions I wouldn't be able to type in any character but with the latest Nightly everything works fine. I will mark this issue accordingly. Thank you.

Yeah, could be a bug of Mozc. I tested with latest Nightly, but I see suggest window properly.

Comment on attachment 9097258 [details]
Bug 1583766 - Consider IMEContentObserver to send selection change notification when its sender sends a text change notification

Fix for perf regression in 70, verified in Nightly.
OK for uplift for beta 13.

Attachment #9097258 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1592626
No longer blocks: 1592626
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.