If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Do not create new functions for every kinetic pan

RESOLVED WONTFIX

Status

Fennec Graveyard
Panning/Zooming
RESOLVED WONTFIX
7 years ago
5 years ago

People

(Reporter: stechz, Assigned: mbrubeck)

Tracking

({perf})

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Due to paranoia from bug 631951, I was worried that we may be compiling Javascript functions for every kinetic pan we were making when we turn methodjit on. It was faster to make a cleanup patch than asking anyone if this could be problematic. :)
(Reporter)

Comment 1

7 years ago
Created attachment 510307 [details] [diff] [review]
Do not create new functions for every kinetic pan
(Reporter)

Updated

7 years ago
Attachment #510307 - Flags: review?(mark.finkle)
(Assignee)

Updated

7 years ago
Keywords: perf
(Assignee)

Updated

6 years ago
Assignee: nobody → mbrubeck
(Assignee)

Comment 2

6 years ago
Created attachment 565128 [details] [diff] [review]
patch v2

This patch was badly bit-rotted.  This updates Ben's approach to work against the current code, and again prevent us from creating new objects or functions on each kinetic pan.
Attachment #510307 - Attachment is obsolete: true
Attachment #565128 - Flags: review?(mark.finkle)
Attachment #510307 - Flags: review?(mark.finkle)
(Assignee)

Comment 3

6 years ago
Created attachment 565129 [details] [diff] [review]
patch v2 (diff -w)

Here's a "diff -w" output, to make it easier to separate the real changes from the indentation changes.
Comment on attachment 565128 [details] [diff] [review]
patch v2

The priginal worry was related to methodjit, which is off in chrome now. Inany case, this shouldn't hurt.
Attachment #565128 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 5

6 years ago
Created attachment 565131 [details] [diff] [review]
s/self/this/g

This mechanical patch removes the now-unnecessary "self" local and replaces it with "this" inside onBeforePaint.  I'll fold this into the main patch before pushing it; just keeping it separate for now to keep reviews easier.
(Assignee)

Comment 6

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #4)
> The priginal worry was related to methodjit, which is off in chrome now.
> Inany case, this shouldn't hurt.

True, though this should at least reduce the amount of garbage collection we do.  It will also help if we manage to fix the methodjit memory usage enough to turn it back on.
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1d5e94097a Thanks Ben!
Status: NEW → ASSIGNED
Whiteboard: [pushed]
Target Milestone: --- → Firefox 10

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/ac1d5e94097a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

6 years ago
Backed out because this broke kinetic panning on repeated swipes:
https://hg.mozilla.org/mozilla-central/rev/737e0ebf68b2

I'll go back and see what I messed up...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [pushed]
Target Milestone: Firefox 10 → ---
(Assignee)

Comment 10

5 years ago
This code is going away in favor of AsyncPanZoomController.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.