284469 - Calling ProcessNodeBind unnecessarily

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

Right now we'll call ProcessNodeBind for all nsXFormsContrlStub controls whether
they have any kind of binding attribute or not.  It causes a lot of unnecessary
processing during document load.

Comment 1

[aaronr]

I'm going to set a flag on the nsXFormsControlStub::kElementFlags during
::AttributeSet if a binding attribute is getting set or removed.  Then we can
check for that flag inside each control's ::Bind (or in the default
nsXFormsControlStub::ResetBoundNode) and only continue on if that flag is set. 
Seems to cut down on the calls to ProcessNodeBind by 50-75% in some of my testcases.

Comment 2

[aaronr]

Attachment: first attempt

This works for me.

Comment 3

[aaronr]

Attachment: second attempt

fixed output element problem.  Also changed repeat element to seperate
::Refresh from ::Bind so that ::Refresh doesn't get called twice in some
instances (like via nsXFormsControlStub::DocumentChanged).

I still need to add RemoveAttribute handling so that the flag properly reflects
the element's state.

Comment 4

[aaronr]

Attachment: added attribute removal support

probably not ready for prime time, yet.  But contains support for attribute
removal on all controls.

Comment 5

[Allan Beaufour]

(In reply to comment #4)
> Created an attachment (id=176310) [edit]
> added attribute removal support
> 
> probably not ready for prime time, yet.  But contains support for attribute
> removal on all controls.

Does this patch correctly handle having a <output bind="b1" ref="n1"/>, and then
remove either of the attributes? Moving @bind should make the output use @ref
for binding instead, and removing @ref should do nothing.

Comment 6

[aaronr]

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=176310) [edit] [edit]
> > added attribute removal support
> > 
> > probably not ready for prime time, yet.  But contains support for attribute
> > removal on all controls.
> 
> Does this patch correctly handle having a <output bind="b1" ref="n1"/>, and then
> remove either of the attributes? Moving @bind should make the output use @ref
> for binding instead, and removing @ref should do nothing.

Nope, not yet.  My next (hopefully final) patch should handle that.

Comment 7

[aaronr]

Attachment: handles attribute removal and rebinding properly

this should be pretty close.  Only extra thing that I could think of was
somehow merging the bind/ref attribute detection into a central function, but
then we'd end up having to execute one more GetAttribute than we really need to
and it would add another method to all controls.  But it would clean up the
code a bunch if I did do it.

Also, I don't know what we should do with the MDG.  If a control has a bind
that makes it irrelevant and we remove that bind, then it should be relevant,
right?	But how should we toggle the attribute on the control?	Something for
me to think about tomorrow.

Comment 8

[aaronr]

Attachment: testcase using various controls

testcase with repeat, input, output, select and trigger.

Comment 9

[aaronr]

Attachment: hopefully final patch

This latest patch includes removing the MIP attributes on the control.

Comment 10

[aaronr]

Attachment: updated testcase

updated testcase.  Note that the checkbox turning into an input field but not
accepting input is a known problem.  Doron is opening a bug on it, probably

Comment 11

[Allan Beaufour]

Comment on attachment 178982 [details] [diff] [review]
hopefully final patch

>Index: nsXFormsControlStub.cpp
>===================================================================

Boy, there is a lot of duplicated code here.

> NS_IMETHODIMP
> nsXFormsControlStub::AttributeSet(nsIAtom *aName, const nsAString &aValue)
> {
>+  // have to make sure that ELEMENT_WITH_SNB_ATTR flag stays accurate if we
>+  // are being told our single node binding attributes are changing.
>+
>+  PRBool bindAndRefresh = PR_FALSE;
>+  if (aName == nsXFormsAtoms::bind ) {
>+    PRUint32 flags = GetElementFlags();
>+    bindAndRefresh = PR_TRUE;
>+    if (!aValue.IsEmpty()) {
>+      flags |= nsXFormsUtils::ELEMENT_WITH_SNB_ATTR;
>+    } else { 
>+      nsAutoString attrStr;
>+      mElement->GetAttribute(NS_LITERAL_STRING("ref"), attrStr);
>+      if (attrStr.IsEmpty()) {
>+        flags &= ~nsXFormsUtils::ELEMENT_WITH_SNB_ATTR;
>+        ResetProperties();
>+      }
>+    }
>+    SetElementFlags(flags);
>+  } else if (aName == nsXFormsAtoms::ref) {
>+    PRUint32 flags = GetElementFlags();
>+    bindAndRefresh = PR_TRUE;
>+    if (!aValue.IsEmpty()) {
>+      flags |= nsXFormsUtils::ELEMENT_WITH_SNB_ATTR;
>+    } else { 
>+      nsAutoString attrStr;
>+      mElement->GetAttribute(NS_LITERAL_STRING("bind"), attrStr);
>+      if (attrStr.IsEmpty()) {
>+        flags &= ~nsXFormsUtils::ELEMENT_WITH_SNB_ATTR;
>+        ResetProperties();
>+      }
>+    }
>+    SetElementFlags(flags);
>+  } else if (aName == nsXFormsAtoms::model) {
>+    // make sure that we run through bind and refresh again if @model added

"@model set"

>+    bindAndRefresh = PR_TRUE;
>+  }
>+      
>+  if (bindAndRefresh) {
>+    Bind();
>+    Refresh();
>+  }
>+
>+  return NS_OK;
>+}

Here is my quick go at it:
{
  if (aName == nsXFormsAtoms::bind || aName == nsXFormsAtoms::ref) {
    PRUint32 flags = GetElementFlags();
    if (!aValue.IsEmpty()) {
      flags |= nsXFormsUtils::ELEMENT_WITH_SNB_ATTR;
    } else { 
      nsAutoString attrStr;
      mElement->GetAttribute(aName == nsXFormsAtoms::bind ?
NS_LITERAL_STRING("ref") : NS_LITERAL_STRING("ref"), attrStr);
      if (attrStr.IsEmpty()) {
	flags &= ~nsXFormsUtils::ELEMENT_WITH_SNB_ATTR;
	ResetProperties();
      }
    }
    SetElementFlags(flags);
  } else if (aName == nsXFormsAtoms::model) {
    // make sure that we run through bind and refresh again if @model set
  } else {
    return NS_OK;
  }

  Bind();
  Refresh();

  return NS_OK;
}


>+NS_IMETHODIMP
>+nsXFormsControlStub::AttributeRemoved(nsIAtom *aName)
>+{

Apply the same tactics as above, and do consider using a common function.

 // nsIXFormsContextControl
@@ -403,8 +497,29 @@ nsXFormsControlStub::GetContext(nsAStrin
   nsCOMPtr<nsIDOMElement> model = do_QueryInterface(mModel);
   if (model) {
     model->GetAttribute(NS_LITERAL_STRING("id"), aModelID);
   }

   return NS_OK;
 }

>+// This function removes all of the attributes that may have been added to the
>+// control due to binding with an instance node.

Move comment to header file.

>+NS_IMETHODIMP
>+nsXFormsControlStub::ResetProperties()

Not an interface-function is it? Return |nsresult|, or rather |void|, since you
are not returning anything.

>+  mElement->RemoveAttribute(NS_LITERAL_STRING("valid"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("invalid"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("enabled"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("disabled"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("required"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("optional"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("read-only"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("read-write"));

:( We should really use atoms... (ignore me, I just hate all the string
comparisons we do)


>Index: nsXFormsControlStub.h
>===================================================================

>+  NS_IMETHOD ResetProperties();

This is not an interface function is it? Then return void.

>+  PRUint32 GetElementFlags() {return mElementFlags;}
>+  void     SetElementFlags(PRUint32 flags) {mElementFlags = flags;}

Add some space between the braces.

>+  /**
>+   * The element flags for the controls passed to
>+   * nsXFormsUtils:EvaluateNodeBinding()
>+   */
>+  PRUint32 mElementFlags;

Add comment about the new usage of the flags.

>Index: nsXFormsOutputElement.cpp
>===================================================================

>+NS_IMETHODIMP
>+nsXFormsOutputElement::AttributeSet(nsIAtom *aName, const nsAString &aValue)
>+{
[...]

Ouch.

How about doing a |nsXFormsControlStrub::AttributeSetInternal(nsIAtom*,
nsAString&, PRBool isOutput)| and doing the value check there?

>+NS_IMETHODIMP
>+nsXFormsOutputElement::AttributeRemoved(nsIAtom *aName)
>+{

[...]

Ouch... same as above.

>+void nsXFormsOutputElement::MaybeRemoveFromModel(nsIAtom *aName)

void on seperate line.

>Index: nsXFormsRepeatElement.cpp
>===================================================================
>+void nsXFormsRepeatElement::MaybeBindAndRefresh(nsIAtom *aName)

void on seperate line.

>+void nsXFormsRepeatElement::MaybeRemoveFromModel(nsIAtom *aName)

void on seperate line.

>Index: nsXFormsUtils.h
>===================================================================

>+    /**
>+     * The element has a "single node binding" attribute (either @bind or @ref)
>+     */

Or @value for output.


I'm not sure your flag-tactics is good. How about storing the SNB-value as a
counter that holds the number of non-empty binding attributes instead? Then you
just need to check whether the value == 0, and count up and down in
willset/set/remove. Would eliminate the |if remove bind, check ref (and
value)|-stuff.

Comment 12

[aaronr]

Da*n, a counter.  MUCH better idea than my flag.  I'll see if I can't rework my
logic using that approach.  Thanks.

Comment 13

[aaronr]

Attachment: patch with Allan's approach

Addresses Allan's concerns with last patch except for all of the
ns_literal_string's in nsXFormsControlStub::ResetProperties.  I didn't know if
we should add atoms for non-spec strings that aren't oft used, like
"read-write".  I can certainly make these into atoms and use f.x.
nsXFormsAtoms::required.ToString() instead.  Just let me know.

Comment 14

[aaronr]

Comment on attachment 179232 [details] [diff] [review]
patch with Allan's approach

oops, forgot the most important part...the test in ResetBoundNode!

Comment 15

[aaronr]

Attachment: updated patch

added the test in ResetBoundNode that makes sure that the binding attr count >
0 before calling ProcessNodeBinding.  I had forgotten to move it when I changed
from the flag approach to the counting approach.

In testcase I just tried (my alphatestcase) only 54 calls to
ProcessNodeBinding, as opposed to 153 before patch.

Comment 16

[Doron Rosenberg (IBM)]

Comment on attachment 179235 [details] [diff] [review]
updated patch

>+void
>+nsXFormsControlStub::ResetProperties()
>+{
>+  if (!mElement) {
>+    return;
>+  }
>+
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("valid"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("invalid"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("enabled"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("disabled"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("required"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("optional"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("read-only"));
>+  mElement->RemoveAttribute(NS_LITERAL_STRING("read-write"));
>+
>+  return;
>+}

That last return is kinda useless, not?

>+
>+void
>+nsXFormsControlStub::AddRemoveSNBAttr(nsIAtom *aName, 
>+                                      const nsAString &aValue)
>+{
>+  nsAutoString attrStr, attrValue;
>+  aName->ToString(attrStr);
>+  mElement->GetAttribute(attrStr, attrValue);
>+
>+  // if we are setting an single node binding attribute that we don't already
>+  // have, bump the count.
>+  if (!aValue.IsEmpty()) {
>+    if (attrValue.IsEmpty()) {
>+      ++mCountBindAttrs;
>+    }
>+  }
Why not if (!aValue.IsEmpty() && attrValue.IsEmpty()) ?

Comment 17

[Allan Beaufour]

Comment on attachment 179235 [details] [diff] [review]
updated patch

>Index: extensions/xforms/nsXFormsControlStub.h
...
>+  void nsXFormsControlStub::MaybeRemoveFromModel(nsIAtom *aName, 
>+                                                 const nsAString &aValue);

*ahem* ...

Comment 18

[Allan Beaufour]

Comment on attachment 179235 [details] [diff] [review]
updated patch

<beaufour:mega-nit-picking>

>Index: extensions/xforms/nsXFormsControlStub.cpp
>===================================================================
...
>+  // if we are setting an single node binding attribute that we don't already

_a_ single node

>+  // have, bump the count.
>+  if (!aValue.IsEmpty()) {
>+    if (attrValue.IsEmpty()) {
>+      ++mCountBindAttrs;
>+    }
>+  }
>+  else if (!attrValue.IsEmpty()) {

move else up


>Index: extensions/xforms/nsXFormsControlStub.h
>===================================================================
...
>+  /**
>+   * This function manages the mCountBindAttrs value.  mCountBindAttrs will
>+   * be incremented if a single node binding attribute is specified that isn't
>+   * currently on the control AND aValue isn't empty.  Otherwise, if the control
>+   * already contains this attribute with a non-empty value and aValue is empty,
>+   * then mCountBindAttrs is decremented.
>+   *
>+   *   aName -  Atom of the single node binding attribute ('bind', 'ref', etc.).

@param aName

>+   *            Using an atom to make it a tad harder to misuse by passing in
>+   *            any old string, for instance.  Since different controls may have
>+   *            different binding attrs, we do NO validation on the attr.  We
>+   *            assume that the caller is smart enough to only send us a
>+   *            binding attr atom.
>+   *   aValue - The string value that the SNB attribute is being set to.

@param aValue

>+  /** 
>+   * Used to keep track of whether this control has any single node binding
>+   * attributes.

Holds the number of non-empty binding attributes on the element. Used to limit
unnecessary refreshes etc. ?

>+   */
>+  PRInt32 mCountBindAttrs;
>+

'mBindAttrsCount' seems to be more natural?

>+
>+  /**
>+   * This function removes all of the attributes that may have been added to
>+   * the control due to binding with an instance node.
>+   */
>+  void ResetProperties();

We know it's a function :)

>+  /**
>+   * This function causes bind() and refresh() to be called if aName is
Bind() and Refresh()

>+   * the atom of a single node binding attribute for this control.  Called by
>+   * AttributeSet and AttributeRemoved.
>+   */
>+  void MaybeBindAndRefresh(nsIAtom *aName);

same.

>+  /**
>+   * Removes this control from its model's list of controls if a single node
>+   * binding attribute is removed.  Called by WillSetAttribute and
>+   * WillRemoveAttribute.
>+   * 
>+   * aName  - atom of the attribute being changed
>+   * aValue - value that the attribute is being changed to.

@param ...

>+   */
>+  void nsXFormsControlStub::MaybeRemoveFromModel(nsIAtom *aName, 
>+                                                 const nsAString &aValue);

</beaufour:mega-nit-picking>

Good patch!

Comment 19

[aaronr]

Attachment: updated with fixed nits

fixed Doron and Allan's nits.  Doron, could you please give 2nd review?

Comment 20

[Doron Rosenberg (IBM)]

Comment on attachment 179983 [details] [diff] [review]
updated with fixed nits

Looks good

Comment 21

[aaronr]

Attachment: updated to trunk

same patch, merged with latest trunk

Comment 22

[Doron Rosenberg (IBM)]

checked in.