Session saver makes typing review comments slow (NOT bug 343659)

RESOLVED FIXED in Firefox 2 beta2

Status

()

Firefox
Session Restore
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: bz, Assigned: Simon Bünzli)

Tracking

({fixed1.8.1, perf})

2.0 Branch
Firefox 2 beta2
fixed1.8.1, perf
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9a2 +
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

BUILD:  Current trunk build; make sure to have a build that includes the changes for bug 343659.

STEPS TO REPRODUCE:
1)  Enable session saver.
2)  Set layout.spellcheckDefault to 0 to avoid bug 344895.
3)  Load https://bugzilla.mozilla.org/attachment.cgi?id=227870&action=edit
4)  Click "Edit Attachment As Comment".
5)  Try typing text into the textarea.

When I do this, I get about one character every 2 seconds.  This will be hardware-dependent, of course; faster machines may not lag quite as much.  If anyone has trouble reproducing, let me know and I'll look for a bigger patch.  ;)

The problem is that session saver gets the .value of the textarea on every single input event.  On my hardware this operation takes about 2 seconds to perform.

The "easy" fix is to fix bug 240933, but that's _so_ not happening for Firefox 2.  So filing this to see whether anyone has any ideas on how to fix this in the meanwhile.  The simplest solution that comes to mind is to only get the value every so often (e.g. set a timeout when an input event happens or something), but that'll still freeze up the app for 2 secs at a time when the value is gotten...
Flags: blocking1.9a2?
Flags: blocking-firefox2?
(Assignee)

Comment 1

12 years ago
Possible solutions:
* Don't get the text at every keypress but just store a reference to the text element and get the value only when we really need it (i.e. in _updateTextAndScrollData).
* Don't observe input at all and simply save the values of all text fields during the update cycle (also in _updateTextAndScrollData). I preferred the other solution because I saw no reason for storing unchanged text data which would be reloaded anyway.

BTW: Both solutions would completely fix your testcase because text values from encrypted sites are per default ignored in _updateTextAndScrollData and not saved at all.
Yes, getting the value only when we're about to save it seems like the right solution to me.
(Assignee)

Comment 3

12 years ago
Created attachment 229540 [details] [diff] [review]
only get the text when we really have to

This implements the first of my suggestions from comment #1.

One more argument against the second suggestion would be that in that case the value will be read for every save operation whereas with this solution it's really only read when we absolutely have to.
Attachment #229540 - Flags: review?(dietrich)
Comment on attachment 229540 [details] [diff] [review]
only get the text when we really have to

r=me

The changes look ok, and the elapsed time per input event went from around 45ms to <1ms.
Attachment #229540 - Flags: review?(dietrich) → review+
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Whiteboard: [has patch][checkin needed]
Target Milestone: --- → Firefox 2 beta2
Assignee: nobody → zeniko
Status: ASSIGNED → NEW
Created attachment 229792 [details] [diff] [review]
patch that applies to current trunk
Checking in nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  n
sSessionStore.js
new revision: 1.31; previous revision: 1.30
done

Checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][checkin needed] → [has patch]

Updated

12 years ago
Flags: blocking1.9a2?
Flags: blocking1.9a2+
Flags: blocking-firefox2?
Flags: blocking-firefox2+
Comment on attachment 229792 [details] [diff] [review]
patch that applies to current trunk

Problem: SS stores text data on every change, causing very poor textarea typing performance in low-resource situations. 

Fix: The data is not needed until the session is saved to disk, so now we only collect the data at that point.

Risk: Low.
Attachment #229792 - Flags: approval1.8.1?
Whiteboard: [has patch] → [has patch][needs approval]
Comment on attachment 229792 [details] [diff] [review]
patch that applies to current trunk

a=drivers. Please land on the MOZILLA_1_8_BRANCH.
Attachment #229792 - Flags: approval1.8.1? → approval1.8.1+
I'm curious why you need to use XPCNativeWrapper explicitly, though.
(Assignee)

Comment 10

12 years ago
(In reply to comment #9)
> I'm curious why you need to use XPCNativeWrapper explicitly, though.

Because they're not automatically applied for JS components (see bug 328159 comment #9).
Whiteboard: [has patch][needs approval] → [checkin needed (1.8 branch)]
mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.5.2.25
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
Version: Trunk → 2.0 Branch
> Because they're not automatically applied for JS components

Was that bug 337095 or something else?  If this is still true, PLEASE file bugs. We want to fix this sort of thing, not just work around it.
(Assignee)

Updated

12 years ago
Depends on: 366302
(Assignee)

Updated

12 years ago
Component: General → Session Restore
QA Contact: general → session.restore
You need to log in before you can comment on or make changes to this bug.