691001 - E4X performance enhancements

Status: RESOLVED FIXED

Description

[Andrew Paprocki]

Simple analysis in profiling tools of "new XML()" being passed a ~500k XML document show a few low-hanging fruit that drastically help reduce CPU time and memory usage. I'll attach patches for the simplest fixes I can see which actually make a big difference in real-world usage.

Comment 1

[Andrew Paprocki]

Created attachment 563931 [details] [diff] [review]
Hoist property check out of tight loop / prevent unnecessary Namespace object creation

This patch addresses two simple issues:

1) XMLToXMLString() is a recursive function that calls GetBooleanXMLSetting as its first order of business to check if pretty-printing is enabled. When toString()-ing a large XML object, this winds up getting called N times where N is the number of E4X objects. This simply hoists the property check up a level so that it is not checked on every recursive call.

2) When GetNamespace() is initially called with a NULL set of ancestor namespaces, the function uses an empty namespace array. This has a side-effect of *never* producing a match for the default namespace assigned to every object (""). Thus, every call to GetNamespace() allocates a *new* Namespace object even though they all have the default namespace string (""). This simply creates a new Namespace object with "" values and inserts it into the empty array object in this case. The end result is that only one Namespace object is created for the entire traversal of the XML document. Previously N Namespace objects would be created for N E4X objects.

Comment 2

[Andrew Paprocki]

Created attachment 563932 [details] [diff] [review]
AppendAttributeValue escapes values without creating transient StringBuffer and JSFlatString

This patch prevents the creation of a transient StringBuffer and a JSFlatString created from it on every AppendAttributeValue when performing toString(). This prevents triple-copying all attribute text as it is written out and memory pressure from doing so.

Comment 3

[Andrew Paprocki]

Created attachment 563933 [details] [diff] [review]
AppendAttributeValue escapes values without creating transient StringBuffer and JSFlatString

Re-upped last patch, missed two NULL->false return value changes.

Comment 4

[Andrew Paprocki]

Here are performance stats to give you an idea of how big a difference just these changes make. I applied these patches and also added this printf as the last thing in ToXMLString() prior to returning: printf("compartment %u\n", cx->compartment->gcBytes); My test loads a ~500k XML document into an object and then calls toString() on it 10 times. When running this test, I am also timing the wall & CPU time.

Without patches:

compartment 6365184
compartment 5775360
compartment 8212480
compartment 4509696
compartment 6946816
compartment 9379840
compartment 5611520
compartment 8044544
compartment 4329472
compartment 6766592
wall 9416.54ms user 9210.00ms

With patches:

compartment 4112384
compartment 4001792
compartment 4186112
compartment 4370432
compartment 3948544
compartment 4132864
compartment 4317184
compartment 4501504
compartment 4079616
compartment 4263936
wall 4689.59ms user 4440.00ms

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 563933 [details] [diff] [review]
AppendAttributeValue escapes values without creating transient StringBuffer and JSFlatString

Looks great, ask #jsapi to land for you -- traveling and super-busy, sorry I can't do it.

/be

Comment 6

[Andrew Paprocki]

Created attachment 569259 [details] [diff] [review]
AppendAttributeValue escapes values without creating transient StringBuffer and JSFlatString.

Re-up patch as export with header for checkin.

Comment 7

[Andrew Paprocki]

Created attachment 569260 [details] [diff] [review]
Hoist property check out of tight loop / prevent unnecessary Namespace object creation.

Re-up patch as export with header for checkin.

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Andrew, in the future could you please include the bug number in the checkin comments?

Pushed:
  http://hg.mozilla.org/integration/mozilla-inbound/rev/2fef1412e0e8
  http://hg.mozilla.org/integration/mozilla-inbound/rev/bdbdde9ab432

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2fef1412e0e8
https://hg.mozilla.org/mozilla-central/rev/bdbdde9ab432

Comment 10

[Alfred Kayser]

Created attachment 569910 [details]
Optimize EscapeAttributeValueBuffer...

Your patch is a great optimization!

Not sure how much time EscapeAttributeValueBuffer takes, but is it worth to optimize EscapeAttributeValueBuffer, so that chunks of non-escaped chars are append in one go?  Appending an array of chars results in a memcpy, but doing one by one with length checks in each s much more costly.

Comment 11

[Andrew Paprocki]

My optimizations were guided by Quantify. After applying the patch it didn't show up as a hotspot. I suppose things can always be made faster but I was just focusing on small wins which were grossly affecting the run time.