Talos has detected a Firefox performance regression from your commit 85383f7a9d53 in bug 1120851. We need you to address this regression. This is a list of all known regressions and improvements related to your bug: http://alertmanager.allizom.org:8080/alerts.html?rev=85383f7a9d53&showAll=1 On the page above you can see Talos 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, please see: https://wiki.mozilla.org/Buildbot/Talos/Tests#TART.2FCART Reproducing and debugging the regression: If you would like to re-run this Talos test on a potential fix, use try with the following syntax: try: -b o -p linux,win64 -u none -t svgr # add "mozharness: --spsProfile" to generate profile data To run the test locally and do a more in-depth investigation, first set up a local Talos environment: https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code Then run the following command from the directory where you set up Talos: talos --develop -e <path>/firefox -a tart Making a decision: As the patch author we need your feedback to help us handle this regression. *** Please let us know your plans by Friday, or the offending patch will be backed out! *** Our wiki page oulines the common responses and expectations: https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling

:m_kato, this causes a significant perf hit on linux/linux64 for a tsvgx/tart, can you comment on why this is happening? We will be uplifting next week, so coming to a decision this week would be ideal.

(In reply to Joel Maher (:jmaher) from comment #1) > :m_kato, this causes a significant perf hit on linux/linux64 for a > tsvgx/tart, can you comment on why this is happening? We will be uplifting > next week, so coming to a decision this week would be ideal. When caret position is moved, we query caret rect, then set candidate / predict window rect by calling GTK API. I will be able to improve it to use nsRunnable. Or if tsvgx and tart don't have focus during test, I don't call caret rect query correct.

> I will be able to improve it to use nsRunnable. Or if tsvgx and tart don't have focus during test, I don't call caret rect query correct. It might not be useful to use nsRunnable... Do you try to post it to current thread? Then, it could be though... # Anyway, in long term strategy, GTK IM module should have a signal to request cursor position (or specific character rect).

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3) > > I will be able to improve it to use nsRunnable. Or if tsvgx and tart don't have focus during test, I don't call caret rect query correct. > > It might not be useful to use nsRunnable... Do you try to post it to current > thread? Then, it could be though... Even if delay by runnable, this wasn't resolved. This root causes is OnLayoutChange. SetCursorPosition can query rect even if no composition after bug 1120851. So we shouldn't query it without composition. But we should update candidate window position if OnLayoutChange is called. So I should call it before gtk_im_context_filter_keypress like Chromium. > # Anyway, in long term strategy, GTK IM module should have a signal to > request cursor position (or specific character rect). Yes. GTK API for IME is 90's style API. We should request modern APIs to them.

Comment on attachment 8626226 [details] [diff] [review] Don't query caret rect on layout changed This regression is by OnLayoutChanged. Even if no composition, we always query caret rect. So I change it before calling gtk_im_context_filter_keypress when needed.

Comment on attachment 8626226 [details] [diff] [review] Don't query caret rect on layout changed Looks good to me. But I don't like mCaretUpdated. Could you rename it to mSetCursorPositionOnKeyEvent or something?

I have verified this on the graph server! Thanks for fixing this.