159314 - Fastload is wasteful wrt nodenames

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The current implementation of Serialize/Deserialize is fairly wastefull with how
it saves names of attributes and elements. It currently writes the full name and
namespace of every element and attribute, which for the xul-namespace is a
fairly long string.

Instead we could start off with writing a table of all name+namespace
combinations we have in the document and then for each element/attribute just
write an index into that table. This is actually very easy to do since the
nodeinfo-manager keeps track of all nodeinfos used in the prototype document so
it's easy to just extract a list out from there.

This will not only save us load-time while loading the fastload file. It will
also save a nodeinfo lookup for every element/attribute since we can do that
just once while loading the table and then just grab the nodeinfo out of the
table. The filesize wins for a fastloadfile after just opening the browser
window is 128kb out of 909kb, approx 14%.


Patch coming up to implement this.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to implement nodeinfo table v1

There are a couple of things i'm not sure what to do with. First of all it
seems kind'a stupid to do a do_QueryElementAt every time I need a nodeinfo out
of the array. An alternative would be to just use an nsVoidArray and just
release by hand when deserialization is done.

I also wasn't sure in which way to modify the fastload-version field. Did
someone have a specific plan with this? Not that it really matters though :)


Oh, and i hope that all files should be in the diff, cvs is acting up on me at
the moment so let me know if anything seems to be missing.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 92700 [details] [diff] [review]
Patch to implement nodeinfo table v1

+nsNodeInfoManager::GetNodeInfoArray(nsISupportsArray** aArray)
+{
+  *aArray = 0;

nsnull when dealing with pointers, not 0! This is not transfomiix code :-)

+  PRUint32 n;
+  PL_HashTableEnumerateEntries(mNodeInfoHash,
+				GetNodeInfoArrayEnumerator,
+				array);
+  array->Count(&n);
+  NS_ENSURE_TRUE(n == mNodeInfoHash->nentries, NS_ERROR_FAILURE);

AFAICT the only case where this would happen is if we're OOM, so return
NS_ERROR_OUT_OF_MEMORY here in stead. And why declare |n| before enumerating
the hash when it's only used after?

+  *aArray = array;
+  NS_ADDREF(*aArray);
+  return NS_OK;
+}

The norm in this file is to always have an empty line before a return, so add
an empty line...

+
+
+PRIntn
+nsNodeInfoManager::GetNodeInfoArrayEnumerator(PLHashEntry* he, PRIntn i,
+					       void* arg)

Why two empty lines before this method when you're removing an empty line
before a method above this one to make those methods be separated by only one
empty line?

Also, add "// static" on the line above the return type here, to make it
obvious that this is a static method.

+  nsresult rv = array->AppendElement((nsINodeInfo*)he->value);
+  return NS_SUCCEEDED(rv) ? HT_ENUMERATE_NEXT : HT_ENUMERATE_STOP;

Wouldn't this be more readable here?:

+  if (NS_FAILED(rv)) {
+    return HT_ENUMERATE_STOP;
+  }
+
+  return HT_ENUMERATE_NEXT;

Other than those nits, I'm fine with the node info changes.

Comment 3

[Axel Hecht]

> +{
> +  *aArray = 0;
>
> nsnull when dealing with pointers, not 0! This is not transfomiix code :-)

well, not if you have scc as sr. He's going for 0 instead of nsnull.

<peterv> this is war

(if it really is, we definitly need a second religion here)

Comment 4

[Johnny Stenback (:jst)]

Well, scc can say whatever he wants (no disrespect or anything) :-) but nearly
all of the mozilla/content code (and most of the other mozilla code as well)
uses nsnull when settign/comparing a pointer to 0, so unless you want to go
replace all nsnull's in this code with 0, stick to using nsnull! IMNSHO using
nsnull increases the readability of the code so I'm lobbying for using nsnull
over 0, and given that their meaning is identical to the compiler I don't see
what the benefit of using 0 would be.

;-)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to implement nodeinfo table v2

Fixes jsts comments

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I think this will be big enough win to want for 1.2alpha, and since there is a
patch on its way this should be doable.

Comment 7

[John Morrison]

The fastload version define started out in nsXULDocument.cpp and the practice
has been to increase the subtractor when the version changes, as you've already 
done in your patch. See rev 1.417 and rev 1.445 of 
content/xul/document/src/nsXULDocument.cpp.

(It does sorta matters, although, yeah, timestamp changes to the jar files
largely prevent a user from using an incompatible version.)

Comment 8

[David Hyatt]

Comment on attachment 93333 [details] [diff] [review]
Patch to implement nodeinfo table v2

sr=hyatt

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 93333 [details] [diff] [review]
Patch to implement nodeinfo table v2

r=jst

Comment 10

[Peter Van der Beken [:peterv]]

Wasn't this checked in?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

yep, this is checked in. It gave a 2-3% decrease in startup-time!

Comment 12

[shrirang khanzode]

.