Bug 43462 - Complex MathML test takes over 20s to render
Summary: Complex MathML test takes over 20s to render
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Alex Milowski
URL: http://www.w3.org/Math/testsuite/mml2...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-03 20:12 PDT by Jon
Modified: 2010-10-21 03:08 PDT (History)
5 users (show)

See Also:


Attachments
Example 1 - Slowest (3.82 KB, application/xhtml+xml)
2010-10-15 17:47 PDT, Alex Milowski
no flags Details
Example 2 - A bit faster (1.87 KB, application/xhtml+xml)
2010-10-15 17:48 PDT, Alex Milowski
no flags Details
Example 3 - Faster - still has a delay (1.54 KB, application/xhtml+xml)
2010-10-15 17:49 PDT, Alex Milowski
no flags Details
Fix for long layout time (200.41 KB, patch)
2010-10-16 14:41 PDT, Alex Milowski
no flags Details | Formatted Diff | Diff
Updated patch with latest test results (28.77 KB, patch)
2010-10-17 14:33 PDT, Alex Milowski
no flags Details | Formatted Diff | Diff
Updated patch with latest optimizations for layout (226.32 KB, patch)
2010-10-18 20:17 PDT, Alex Milowski
kenneth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Updated patch that conforms with the changes to baselinePosition (226.44 KB, patch)
2010-10-20 14:03 PDT, Alex Milowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon 2010-08-03 20:12:25 PDT
Running this W3 MathML2 test on my i7 iMac results in a beachball on ToT WebKit, eventually resolving after 15 - 20 seconds. I'll try again later and get some samples.
Comment 1 François Sausset 2010-08-04 01:40:07 PDT
The MathML implementation in WebKit is just at its early stage, so no optimization work was really done.
We still have a lot of work on layout issues and I think we should next focus on optimizations.

If you could isolate the bottleneck in this example and build a test case, it would be very useful!
Comment 2 Alex Milowski 2010-10-15 15:53:40 PDT
I think this has actually gotten worse.  I can't get the page to render after a couple of minutes on a fast machine.
Comment 3 Alex Milowski 2010-10-15 17:46:37 PDT
Nesting rows (actual or implied) more than 9 levels causes serious performance problems.  This has to do with the layout algorithm from RenderMathMLRow.  The multiple calls to layout() cause a cascade of layouts that multiply as the levels increase.
Comment 4 Alex Milowski 2010-10-15 17:47:37 PDT
Created attachment 70931 [details]
Example 1 - Slowest
Comment 5 Alex Milowski 2010-10-15 17:48:15 PDT
Created attachment 70932 [details]
Example 2 - A bit faster
Comment 6 Alex Milowski 2010-10-15 17:49:03 PDT
Created attachment 70933 [details]
Example 3 - Faster - still has a delay
Comment 7 Alex Milowski 2010-10-16 14:41:36 PDT
Created attachment 70965 [details]
Fix for long layout time

This patch changes the row layout algorithm so it doesn't recurse into rows multiple times.
Comment 8 Alex Milowski 2010-10-17 10:37:14 PDT
This bug is a high priority as the row layout algorithm is central to MathML and it is currently very broken from a performance perspective.
Comment 9 Alex Milowski 2010-10-17 14:33:06 PDT
Created attachment 70983 [details]
Updated patch with latest test results
Comment 10 Alex Milowski 2010-10-18 20:17:21 PDT
Created attachment 71125 [details]
Updated patch with latest optimizations for layout
Comment 11 Kenneth Rohde Christiansen 2010-10-20 07:47:40 PDT
Comment on attachment 71125 [details]
Updated patch with latest optimizations for layout

View in context: https://bugs.webkit.org/attachment.cgi?id=71125&action=review

Generally looks good. It actually seems to simplify the code. I notice that in simple fractions, the denominator almost touches the line. I do not know if this is a regression, but maybe this be improved?

> WebCore/mathml/RenderMathMLRow.cpp:79
> +            // Check to see if the non-operator block has a greater height

Please add a dot at the end of comments, that is out webkit style
Comment 12 WebKit Commit Bot 2010-10-20 07:55:18 PDT
Comment on attachment 71125 [details]
Updated patch with latest optimizations for layout

Rejecting patch 71125 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71125]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=71125&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=43462&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 71125 from bug 43462.
Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4524014
Comment 13 Alex Milowski 2010-10-20 08:01:25 PDT
(In reply to comment #11)
> (From update of attachment 71125 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71125&action=review
> 
> Generally looks good. It actually seems to simplify the code. I notice that in simple fractions, the denominator almost touches the line. I do not know if this is a regression, but maybe this be improved?

As the layout algorithm for row is now less agressive, the operators won't keep adding to the row height.  In some cases, we'll need to add extra padding.  I'll look at this a file a separate bug if we need to do that for fractions.
Comment 14 Alex Milowski 2010-10-20 08:03:14 PDT
(In reply to comment #12)
> (From update of attachment 71125 [details])
> Rejecting patch 71125 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71125]" exit_code: 2
> Cleaning working directory
> Updating working directory
> Logging in as commit-queue@webkit.org...
> Fetching: https://bugs.webkit.org/attachment.cgi?id=71125&action=edit
> Fetching: https://bugs.webkit.org/show_bug.cgi?id=43462&ctype=xml
> Processing 1 patch from 1 bug.
> Processing patch 71125 from bug 43462.
> Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1
> 
> Full output: http://queues.webkit.org/results/4524014

The output is just the above.  What does this mean?  Where's the error?
Comment 15 Alex Milowski 2010-10-20 08:43:39 PDT
The patch fails due to a change in the baselinePosition() method signature.  I still don't understand where the error output from the svn-apply went to as that would have told me that in the bug comment.
Comment 16 Alex Milowski 2010-10-20 14:03:25 PDT
Created attachment 71334 [details]
Updated patch that conforms with the changes to baselinePosition
Comment 17 WebKit Commit Bot 2010-10-21 03:08:32 PDT
Comment on attachment 71334 [details]
Updated patch that conforms with the changes to baselinePosition

Clearing flags on attachment: 71334

Committed r70221: <http://trac.webkit.org/changeset/70221>
Comment 18 WebKit Commit Bot 2010-10-21 03:08:40 PDT
All reviewed patches have been landed.  Closing bug.