537289 - Only save form data for fields that aren't the default value

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Instead of saving the value of every field on the page (which slows us down), we should be able to save the value for fields that have changed from the default.

This should give us considerable gains for the majority of "large" forms which have been causing problems (cf. bug 477564, bug 536910)

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1 (WIP)

Simon: I'm cheating to get you to look at this - it seems like the best way to get your attention :) I still need to write tests for this.

Does what it's supposed to do, at least it should. It currently passes the form saving tests we have, but we should have some more extensive ones to make sure this works properly

I want to say it has lower overhead from attribute checking (per bug 537290), but it doesn't since I'm checking defaults as well. I tried to use as few attributes as possible and save them when I could.

Thoughts:
* I don't have a check for non-multiple <select>s. If no option is specified as selected, the first one is selected (so node.selected == true but node.defaultSelected == false). We'd end up saving anyway. I guess it'd still be worth doing though since there can be 
* The comments about <select>s aren't exactly correct...

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

FWIW, Boris profiled with this and the patch in bug 537290 applied and the test case in bug 536910. In the case where no checkboxes were checked, it's a 97% improvement. Toggling several checkboxes high on the page was still ~95% better, and toggling some at the bottom was ~80% better.

While this patch won't help in the case where they are all checked (in fact it will hurt marginally as there's 1 extra attribute lookup), the patch in bug 537290 more than offsets that so we should still win.

Comment 3

[Simon Bünzli]

Comment on attachment 419618 [details] [diff] [review]
Patch v0.1 (WIP)

Overall this looks like an alright short-term fix for part of the XPath performance mess. Comments on the code:

>+      let nType = node.type;

You need the type at most once, so there's no need to cache it, is there?

>+      // Don't increment here because we don't know if we're going to be storing data.

That comment won't make sense once the patch has been checked in.

>+            hasDefaultValue = ((value = node.checked) == node.defaultChecked);

Please make this two lines.

>+            //XXXzpao Alternative is assume fileList is always empty by default, like so:
>+            // hasDefaultValue = !value.fileList.length;

AFAICT the file list has to be empty for security reasons anyway (so a web page can't automatically upload random files), so this alternative should be fine.

>+      if (!hasDefaultValue) {

Please add this bug's number in a comment so we know later that we don't save default values mainly due to XPath generation being slow.

>+        if (!nId)
>+          generatedCount++;
>+        data[nId ? "#" + nId : XPathHelper.generate(node)] = value;

And here it'd be cleaner to drop the ternary ?: and add an "else" instead.

(In reply to comment #1)
> Simon: I'm cheating to get you to look at this - it seems like the best way to
> get your attention :)

As long as I get an email... Any other bug I should have a look at?

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #3)
> (From update of attachment 419618 [details] [diff] [review])
> Overall this looks like an alright short-term fix for part of the XPath
> performance mess. Comments on the code:
> 
> >+      let nType = node.type;
> 
> You need the type at most once, so there's no need to cache it, is there?

True.

> >+      // Don't increment here because we don't know if we're going to be storing data.
> 
> That comment won't make sense once the patch has been checked in.

True.

> >+            // hasDefaultValue = !value.fileList.length;
> 
> AFAICT the file list has to be empty for security reasons anyway (so a web page
> can't automatically upload random files), so this alternative should be fine.

Ok. That's what I thought, but wanted a 2nd opinion to be sure.

> >+      if (!hasDefaultValue) {
> 
> Please add this bug's number in a comment so we know later that we don't save
> default values mainly due to XPath generation being slow.

Sure thing.

> >+        if (!nId)
> >+          generatedCount++;
> >+        data[nId ? "#" + nId : XPathHelper.generate(node)] = value;
> 
> And here it'd be cleaner to drop the ternary ?: and add an "else" instead.

Ok. I was just trying to cut down on lines.

> > Simon: I'm cheating to get you to look at this - it seems like the best way to
> > get your attention :)
> 
> As long as I get an email... Any other bug I should have a look at?

Not that I can think of right now. I feel like I've asked a couple questions in the past that have gone unanswered (but I could be misremembering). If I see any or it happens in the future, I'll make sure to shoot you an email.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Updated my patch but realized we're going to have to update the test for bug 456342, which as written is going to ignore all fields in the form, regardless of their type.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Addresses comments & adjusts the test for bug 456342 (the easy way).

Comment 7

[Simon Bünzli]

Comment on attachment 433201 [details] [diff] [review]
Patch v0.2

>+          case "checkbox":
>+          case "radio":
>+            hasDefaultValue =
>+              (value = node.checked) == node.defaultChecked;

Nit: These two lines fit on a single one (same for the default case). And if you want to make the inner assignment more obvious, rather split this into two statements.

>+        // <select> without multiple is hard to determine default, assume it's not

Nit: grammar.

r+=me with these nits fixed.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/b25b7ed1e55a with nits from comment #7 addressed.