Last Comment Bug 691001 - E4X performance enhancements
: E4X performance enhancements
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Andrew Paprocki
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-30 21:46 PDT by Andrew Paprocki
Modified: 2011-10-27 08:39 PDT (History)
3 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Hoist property check out of tight loop / prevent unnecessary Namespace object creation (4.40 KB, patch)
2011-09-30 21:53 PDT, Andrew Paprocki
no flags Details | Diff | Review
AppendAttributeValue escapes values without creating transient StringBuffer and JSFlatString (4.55 KB, patch)
2011-09-30 22:15 PDT, Andrew Paprocki
no flags Details | Diff | Review
AppendAttributeValue escapes values without creating transient StringBuffer and JSFlatString (4.55 KB, patch)
2011-09-30 22:19 PDT, Andrew Paprocki
brendan: review+
Details | Diff | Review
AppendAttributeValue escapes values without creating transient StringBuffer and JSFlatString. (4.83 KB, patch)
2011-10-24 18:48 PDT, Andrew Paprocki
andrew: review+
Details | Diff | Review
Hoist property check out of tight loop / prevent unnecessary Namespace object creation. (4.68 KB, patch)
2011-10-24 18:48 PDT, Andrew Paprocki
andrew: review+
Details | Diff | Review
Optimize EscapeAttributeValueBuffer... (1.53 KB, text/plain)
2011-10-27 00:44 PDT, Alfred Kayser
no flags Details

Description Andrew Paprocki 2011-09-30 21:46:06 PDT
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 2011-09-30 21:53:19 PDT
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 2011-09-30 22:15:25 PDT
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 2011-09-30 22:19:07 PDT
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 2011-09-30 22:50:32 PDT
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] 2011-10-09 14:12:30 PDT
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 2011-10-24 18:48:01 PDT
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 2011-10-24 18:48:55 PDT
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 [:bz] 2011-10-26 05:54:40 PDT
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 10 Alfred Kayser 2011-10-27 00:44:51 PDT
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 2011-10-27 08:39:15 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.