Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#12450 closed defect (fixed)

Dojo Parser Performance

Reported by: Dustin Machi Owned by: bill
Priority: high Milestone: 1.7
Component: Parser Version: 1.6.0
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

It was recently brought to our attention that there was a performance regression between 1.4 and 1.5 of Dojo. The difference is pretty severe as the dom grows larger. On Chrome performance goes from ~850ops/sec to 35ops sec, ~430 to 3 on ff, and 11 to .25 on IE. (one op = dojo.parser.parse() against a large dom).

You can see versions of the test against 1.4.3, 1.5.0, and 1.6.0 at these urls:

I've discussed a with Bill and he identified one lang issue that is ~3x performance improvement, but its still a pretty far cry from what it was. I'm opening this ticket to discuss what we can do about it.

Attachments (1)

parserPerf.patch (98.7 KB) - added by bill 8 years ago.
changes to parser to optimize dir/lang stuff (while still doing top-down parse), plus dustin's performance test file

Download all attachments as: .zip

Change History (14)

comment:1 Changed 8 years ago by Dustin Machi

Note the discussion comes from changes in http://trac.dojotoolkit.org/ticket/10402

comment:2 Changed 8 years ago by Eugene Lazutkin

Milestone: tbd1.6.1
Owner: changed from anonymous to Eugene Lazutkin
Status: newassigned

comment:3 Changed 8 years ago by bill

Component: GeneralParser
Description: modified (diff)
Milestone: 1.6.11.7

Thanks for looking at this. Here's some background on the issues:

The change was made to:

  1. Support stopParser: true flag in ContentPane and a few other widgets. That flag stops the parser from parsing the subtree below the widget, allowing ContentPane to instead parse it's content at will... and that it turn solved a few bugs like Menu widgets not getting destroyed properly when the ContentPane containing them was destroyed.
  2. Support dir and lang attributes which can get inherited from any ancestor node (even one without a dojoType/data-dojo-type declaration).
  3. avoid dojo.query() dependency

Regarding performance:

  1. The performance slow-down is with parsing a large DOM with little or no dojoType (or data-dojo-type) declarations. When I made the changes in 1.5, I tested on themeTester.html and benchTool.html and there wasn't any significant performance change because the widget instantiation time dwarfed the parse search time.
  1. Performance will likely vary between browsers. A non-recursive search for data-dojo-type is faster for browsers with querySelectorAll() vs. old versions of IE without it. I was concentrating on IE since browsers like chrome are so fast regardless.
  1. My thinking is that supporting (1) and (2) without recursion will make the parser run at O(n log n) rather than O(n), since in the common case, for every node with data-dojo-type/dojoType specified you will need to trace up the DOM tree all the way back to <body> looking for widgets with stopParser: true defined or nodes with dir or lang specified.

Of course, actual performance will vary depending on what percentage of nodes have a data-dojo-type specified, and the fan-out of the DOM tree. But my goal with the current design was to keep performance at O(n) in all cases.

I'll attach the patch I had to avoid some computations about dir/lang. It still runs recursively though.

Also, I'm changing the milestone to 1.7 since 1.6.1 should be reserved mainly for fixing regressions from 1.5 to 1.6, whereas this performance issue has been there since 1.5. I don't really care though if you want to check into the 1.6.1 branch.

Changed 8 years ago by bill

Attachment: parserPerf.patch added

changes to parser to optimize dir/lang stuff (while still doing top-down parse), plus dustin's performance test file

comment:4 Changed 8 years ago by bill

(In [24126]) Optimize code, removing recursion and deferring calculation of dir/lang/textDir until needed (but still caching the results to avoid O(n log n) behavior). Still running as top down parser. Refs #12450, #10402 !strict.

comment:5 Changed 8 years ago by bill

Owner: changed from Eugene Lazutkin to bill
Status: assignednew

Given no other movement on the ticket, I'm going to assume my checkin fixed this.

Last edited 8 years ago by bill (previous) (diff)

comment:6 Changed 8 years ago by bill

Resolution: fixed
Status: newclosed

comment:7 Changed 7 years ago by bill

In [27673]:

add parser performance test, refs #10402, #12450

comment:20 Changed 7 years ago by bill

In [27675]:

tweak parser performance test, refs #12450

comment:21 Changed 7 years ago by bill

In [27699]:

more realistic test, refs #12450

comment:22 Changed 7 years ago by bill

In [27704]:

Fix typo in test, it wasn't really testing _stopParser: true before. Created two separate tests, one widget-heavy test with _stopParser: true and the other with _stopParser: false. Refs #12450.

comment:23 Changed 7 years ago by bill

In [27730]:

Use the actual performance test from the ticket, refs #12450.

comment:24 Changed 7 years ago by bill

In [27740]:

Clear cached data between benchmark runs, for accurate results. Doesn't seem to change numbers though. Refs #12450 !strict.

comment:25 Changed 6 years ago by bill

In [30472]:

benchmarks should not be isDebug:true, especially when using Deferreds, refs #12450

Note: See TracTickets for help on using tickets.