Opened 10 years ago

Closed 9 years ago

Last modified 7 years ago

#10402 closed task (fixed)

top down parser

Reported by: bill Owned by: bill
Priority: high Milestone: 1.5
Component: Dijit Version: 1.4.0b
Keywords: Cc: Eugene Lazutkin
Blocked By: Blocking:

Description (last modified by bill)

Investigate parsing via direct DOM traversal (DFS) instead of using dojo.query().

Initial tests show <= 20ms on FF, Safari, and IE for finding all the dojoType DOM Nodes in themeTester.hml, so the performance argument for dojo.query() seems exaggerated.

The reasons for using custom code are:

  • can track dir setting (either rtl or ltr) of each DOM node thus passing appropriate dir parameter to each widget.
  • similar for themes, if we want to define certain sections of the page having certain themes
  • opens the door to more powerful parser, tracking parent/child relationships, see the parser should do more thread on dojo-contributors
  • parser doesn't need to call getParent() to find out which are the top level widgets
  • theoretical performance improvement because there's only a single call to getAttribute("dojoType") for each node
  • parser can collect <script type=dojo/...> nodes during the dojoType searching DOM traversal, thus eliminating the secondary dojo.query() calls, removing the dojo.query() dependency from the parser and also having a theoretical speed increase

Possible related changes:

  • modify parser to only call startup on truly top level widgets, and then modify ContentPane to call startup() on all children (even children w/out a getParent() method).
  • perhaps move _Container.startup() code to _Widget.startup()
  • pass parameter to startup() like {parentWillResize: true} so that child (dijit.layout._LayoutWidget subclass) doesn't need to call getParent() to find out if it should resize() itself during startup()

Attachments (2)

simple.diff (793 bytes) - added by bill 10 years ago.
simple patch to replace dojo.query() call w/DFS search (no API changes), against parser.js@20654
fancy.diff (2.1 KB) - added by bill 10 years ago.
patch to do DFS in parser, starting to change arguments to instantiate() to pass along additional information associated w/each DOM node

Download all attachments as: .zip

Change History (24)

comment:1 Changed 10 years ago by Eugene Lazutkin

Cc: Eugene Lazutkin added

Changed 10 years ago by bill

Attachment: simple.diff added

simple patch to replace dojo.query() call w/DFS search (no API changes), against parser.js@20654

Changed 10 years ago by bill

Attachment: fancy.diff added

patch to do DFS in parser, starting to change arguments to instantiate() to pass along additional information associated w/each DOM node

comment:2 Changed 10 years ago by bill

Description: modified (diff)

comment:3 Changed 9 years ago by bill

(In [21608]) Remove parser dependence on dojo.query() by implementing search for dojoType and <script type="dojo/..."> using a simple recursive algorithm.

The signature to dojo.parser.instantiate() is changed, but it supports the old signature (too) for backwards compatibility. Note that when used w/the old signature it still calls dojo.query() to find the <script> nodes.

Performance change is unclear: the time for themeTester.html to find all the dojoType nodes is very fast, even on IE6, regardless of the algorithm (16ms or 32ms).

Refs #10402 !strict.

comment:4 Changed 9 years ago by bill

(In [21609]) Enhance parser to track dir=ltr/rtl settings on nodes (with or without a dojoType setting), and propogate those settings down to descendant widgets when the widgets are constructed.

Refs #10402, #10881.

comment:5 Changed 9 years ago by Mark Wubben

I have a PrerenderedWidget class that, if the widget has widgetsInTemplate: true, runs dojo.parse() on the widget's srcNodeRef. So:

  • dojo.ready() triggers a dojo.parse() for the entire document
  • This instantiates a PrerenderedWidget
  • This widget parses its subtree for nested widgets
  • The original dojo.parse() continues instantiating the nested widgets that already were instantiated by the PrerenderedWidget
  • The nested widget is instantiated twice (or, rather, two widgets are instantiated based on one DOM node)

Line 160 in parser.js checks if the node (still?) has a dojoType. If this would also test for node.parentNode that'd solve the problem.

Alternatively, with the new top-down parsing, it might make sense to give widgets the capability to parse their own subtree and stop the top-down parsing if this is the case. The recurse() function would have to resolve the class info, rather than instantiate.

P.S. I can't quite pinpoint what changed in the parser to make this problem occur. It wasn't earlier though.

P.P.S. How do stack size limitations affect the parsing? Safari 3 has a limit of 100, for example: <http://novemberborn.net/2007/08/javascriptcallstack-size-120>.

comment:6 Changed 9 years ago by Mark Wubben

And to handle the case of PrerenderedWidgets containing another PrerenderedWidget, I also had to test for node.getAttribute("widgetid").

comment:7 Changed 9 years ago by bill

Resolution: fixed
Status: newclosed

(In [21651]) Add "inherited" parameter to parser to specify default dir/lang for created widgets.

Make ContentPane use that parameter so widgets loaded via href inherit dir and lang from the ContentPane itself.

Also, made _Templated's widgetsInTemplate passes down widget's dir/lang to the widgets in the template.

Fixes #10402 !strict.

comment:8 in reply to:  5 ; Changed 9 years ago by bill

Replying to Mark Wubben:

  • The original dojo.parse() continues instantiating the nested widgets that already were instantiated by the PrerenderedWidget
  • The nested widget is instantiated twice (or, rather, two widgets are instantiated based on one DOM node)

I'm not surprised it doesn't work, but I can't see how it worked before either. I guess it was a fluke.

Alternatively, with the new top-down parsing, it might make sense to give widgets the capability to parse their own subtree and stop the top-down parsing if this is the case. The recurse() function would have to resolve the class info, rather than instantiate.

Yes, I was thinking of having that in the future. I'd like to do it for things like deferred parsing on tabs (waiting until the tab is displayed) although I figured I'd have to wait until 2.0 because of a hairline backwards compatibility problem.

But I could add a flag now for your case, I guess it would be a flag in the prototype of the specified class (PrerenderedWidget?.prototype in your example). Or it could be a flag on the DOMNode itself, but a flag in the widget seems more useful... what do you think?

P.P.S. How do stack size limitations affect the parsing? Safari 3 has a limit of 100, for example: <http://novemberborn.net/2007/08/javascriptcallstack-size-120>.

Hopefully that won't be an issue; I can't imagine HTML nesting of 100 levels (it's hard to imagine even 10 levels), and I also assume that safari 4 has a bigger stack limit. If the issue comes up though we'll have to deal with it.

comment:9 in reply to:  8 Changed 9 years ago by Mark Wubben

Replying to bill:

But I could add a flag now for your case, I guess it would be a flag in the prototype of the specified class (PrerenderedWidget?.prototype in your example). Or it could be a flag on the DOMNode itself, but a flag in the widget seems more useful... what do you think?

Yes, flag on the prototype. parseSubtree: false? (with a !== false test).

Hopefully that won't be an issue; I can't imagine HTML nesting of 100 levels (it's hard to imagine even 10 levels), and I also assume that safari 4 has a bigger stack limit. If the issue comes up though we'll have to deal with it.

Given that it's an issue in older browsers (stack limits are now in the thousands generally) I'm not sure how quickly we'd run into it. Haven't done the research to see if it's an issue in Dojo-supported browsers either.

comment:10 Changed 9 years ago by bill

(In [21680]) If widget prototype has a "stopParser" flag set, then don't look for widgets declared inside of object. Still looks for <script type=dojo/*> though. Refs #10402 !strict.

comment:11 Changed 9 years ago by bill

Mark - I preliminarily (is that a word?) added a "stopParser" flag, to stop the parser from recursing, similar to what you suggested above. I might change it to something else if I can figure out what the future widget metadata will look like (see the "parser should do more" thread listed above).

I want to point out that a "don't parse subtree" flag isn't appropriate for all pre-rendered widgets. It will work in your _widgetsInTemplate case and the case of simple pre-rendered widgets like a dijit.form.Button. But, it will fail when there are child widgets (like the ContentPane's in a BorderContainer), or when there are internally created widgets (but not via _widgetsInTemplate), like the TabController and TabButton's associated with a TabContainer. In those cases it would be better if the parser recursed.

Then there are hybrid cases like a _widgetsInTemplate widget that also has a containerNode with some child widgets in it. In that case maybe it would be better if the pre-rendered template used a different dojoType flag, for example:

<div dojoType=MyDialog>
        <div dojoAttachPoint=containerNode>
                <div dojoType=dijit.form.TextBox>    <-- want parser to parse this
       </div>
        <div dojoType2=dijit.form.Button>...OK...</div>       <--- but this is handled by _widgetsInTemplate
</div>

Anyway, it's complicated.

comment:12 Changed 9 years ago by bill

(In [21744]) [21680] assumed that all classes are defined before the parser scans the document for nodes with dojoType set. That isn't the case for classes defined via dojo.Declaration.

This is a minimal fix, not sure what the best approach is.

Refs #10402 !strict.

comment:13 Changed 9 years ago by bill

(In [21748]) When parser called on root of document, be sure to get the dir setting of the document itself as the initial inherited dir value. This fixes the order of the icons in test_Toolbar.html?dir=rtl. Refs #10402 !strict.

comment:14 Changed 9 years ago by Mark Wubben

Bill, just updated to HEAD and added the stopParser flag. So far works a treat, thanks!

comment:15 Changed 9 years ago by bill

(In [21837]) Test changes corresponding to [21748], which passes in dir all the time. Refs #10402.

comment:16 Changed 9 years ago by bill

(In [21866]) fix backwards compatibility issue for dojo.parser.instantiate() on nodes that specify type (like <input type=...>), refs #10402, fixes #10951 !strict.

comment:17 Changed 8 years ago by Eugene Lazutkin

If I am to believe to patches, the parser is recursive. Here goes speed.

comment:18 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:19 Changed 7 years ago by bill

In [27673]:

add parser performance test, refs #10402, #12450

comment:20 Changed 7 years ago by bill

In [27703]:

fix typo in comment, refs #10402

comment:21 Changed 7 years ago by bill

In [29935]:

avoid spurious failure due to race condition, refs #10402

comment:22 Changed 7 years ago by bill

In [29936]:

avoid spurious failure due to race condition (on 1.8 branch), refs #10402

Note: See TracTickets for help on using tickets.