850981 - make AppendChild() inline around InsertChildAt()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 2

[Trevor Saunders (:tbsaunde)]

Since you reviewed the oother two I wonder if you just missed this one?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Since you reviewed the oother two I wonder if you just missed this one?

no, no, it just requires some love :)

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 724788 [details] [diff] [review]
patch

Review of attachment 724788 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/Accessible.cpp
@@ -2647,5 @@
> -  if (!aChild)
> -    return false;
> -
> -  if (!mChildren.AppendElement(aChild))
> -    return false;

nsTArray has two different implementations for AppendElement and InsertElementAt. I wonder if it was done on purpose and because of perf issue.

Also we shouldn't null out mEmbeddedObjCollector on AppendChild even if it doesn't have an use case now. (We will need this when we implement smart accessible tree update)

::: accessible/src/generic/Accessible.h
@@ +333,5 @@
>    /**
>     * Append/insert/remove a child. Return true if operation was successful.
>     */
> +  bool AppendChild(Accessible* aChild)
> +  { return InsertChildAt(mChildren.Length(), aChild); }

nit: two space indent for the body

::: accessible/src/generic/BaseAccessibles.h
@@ -34,5 @@
>    // Accessible
>    virtual Accessible* ChildAtPoint(int32_t aX, int32_t aY,
>                                     EWhichChildAtPoint aWhichChild);
>    
> -  virtual bool AppendChild(Accessible* aChild) MOZ_OVERRIDE MOZ_FINAL;

nit: pls remove whitespace above this line

::: accessible/src/mac/AccessibleWrap.mm
@@ +197,5 @@
>    NS_OBJC_END_TRY_ABORT_BLOCK;
>  }
>  
>  bool
> +AccessibleWrap::InsertChildAt(Accessible* aAccessible)

index is lost

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> Comment on attachment 724788 [details] [diff] [review]
> patch
> 
> Review of attachment 724788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/generic/Accessible.cpp
> @@ -2647,5 @@
> > -  if (!aChild)
> > -    return false;
> > -
> > -  if (!mChildren.AppendElement(aChild))
> > -    return false;
> 
> nsTArray has two different implementations for AppendElement and
> InsertElementAt. I wonder if it was done on purpose and because of perf
> issue.

I'm not sure, but adding an if to decide which to call could also hurt perf, and I want to make this stuff easy to understand then we can profile to see what is actually slow.  Also I suspect we'll be using InsertChildAt() directly more than AppendChild() soon.

> Also we shouldn't null out mEmbeddedObjCollector on AppendChild even if it
> doesn't have an use case now. (We will need this when we implement smart
> accessible tree update)

I think that's probably a reasonable optimization, but I don't see why anything would require it given its just a cache.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> > nsTArray has two different implementations for AppendElement and
> > InsertElementAt. I wonder if it was done on purpose and because of perf
> > issue.
> 
> I'm not sure, but adding an if to decide which to call could also hurt perf,
> and I want to make this stuff easy to understand then we can profile to see
> what is actually slow.

I'd recommend to get feedback from someone who knows this code like Neil.

>  Also I suspect we'll be using InsertChildAt()
> directly more than AppendChild() soon.

if you mean smart tree update then it'd be valid for certain js heavy pages only. I'd expect that AppendChild will be used in 90% in either case.

> > Also we shouldn't null out mEmbeddedObjCollector on AppendChild even if it
> > doesn't have an use case now. (We will need this when we implement smart
> > accessible tree update)
> 
> I think that's probably a reasonable optimization, but I don't see why
> anything would require it given its just a cache.

I'd append Cut method for the collector to drop all cache after the index

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > > nsTArray has two different implementations for AppendElement and
> > > InsertElementAt. I wonder if it was done on purpose and because of perf
> > > issue.
> > 
> > I'm not sure, but adding an if to decide which to call could also hurt perf,
> > and I want to make this stuff easy to understand then we can profile to see
> > what is actually slow.
> 
> I'd recommend to get feedback from someone who knows this code like Neil.

I think jlebbar is the conical person for nsTArray, but anyway if your really worried about the perf of this we can add an if now, or just wait and see what the problems are and add one later if its a win.

> >  Also I suspect we'll be using InsertChildAt()
> > directly more than AppendChild() soon.
> 
> if you mean smart tree update then it'd be valid for certain js heavy pages
> only. I'd expect that AppendChild will be used in 90% in either case.

I'd probably believe that a majority of tree updates append children, but I'm not sure we'll be making that happen by calling AppendChild().

> > > Also we shouldn't null out mEmbeddedObjCollector on AppendChild even if it
> > > doesn't have an use case now. (We will need this when we implement smart
> > > accessible tree update)
> > 
> > I think that's probably a reasonable optimization, but I don't see why
> > anything would require it given its just a cache.
> 
> I'd append Cut method for the collector to drop all cache after the index

so your talking about more or less seperate improvements we can make to that cache?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > I'd recommend to get feedback from someone who knows this code like Neil.
> 
> I think jlebbar is the conical person for nsTArray, but anyway if your
> really worried about the perf of this we can add an if now, or just wait and
> see what the problems are and add one later if its a win.

I'm not sure whether we have perf tests for this. Getting feedback shouldn't hurt us, just staying on the safe side. Can you cc him?

> > >  Also I suspect we'll be using InsertChildAt()
> > > directly more than AppendChild() soon.
> > 
> > if you mean smart tree update then it'd be valid for certain js heavy pages
> > only. I'd expect that AppendChild will be used in 90% in either case.
> 
> I'd probably believe that a majority of tree updates append children, but
> I'm not sure we'll be making that happen by calling AppendChild().

how is that?

> > > > Also we shouldn't null out mEmbeddedObjCollector on AppendChild even if it
> > > > doesn't have an use case now. (We will need this when we implement smart
> > > > accessible tree update)
> > > 
> > > I think that's probably a reasonable optimization, but I don't see why
> > > anything would require it given its just a cache.
> > 
> > I'd append Cut method for the collector to drop all cache after the index
> 
> so your talking about more or less seperate improvements we can make to that
> cache?

if you agree then it's the right way then separate bug should be fine (if you wouldn't want to deal with it here)

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > > I'd recommend to get feedback from someone who knows this code like Neil.
> > 
> > I think jlebbar is the conical person for nsTArray, but anyway if your
> > really worried about the perf of this we can add an if now, or just wait and
> > see what the problems are and add one later if its a win.
> 
> I'm not sure whether we have perf tests for this. Getting feedback shouldn't
> hurt us, just staying on the safe side. Can you cc him?

tbh I'd rather just add the if and stop wasting time on this.

the perf tests we do have assuming they work basically just cover tree update.

> > > >  Also I suspect we'll be using InsertChildAt()
> > > > directly more than AppendChild() soon.
> > > 
> > > if you mean smart tree update then it'd be valid for certain js heavy pages
> > > only. I'd expect that AppendChild will be used in 90% in either case.
> > 
> > I'd probably believe that a majority of tree updates append children, but
> > I'm not sure we'll be making that happen by calling AppendChild().
> 
> how is that?

because I'm going to rework CacheChildren() stuff soon so we don't need to use InvalidateChildren() as much, and then its easier to not have worry about if your inserting or appending a child.

> > > > > Also we shouldn't null out mEmbeddedObjCollector on AppendChild even if it
> > > > > doesn't have an use case now. (We will need this when we implement smart
> > > > > accessible tree update)
> > > > 
> > > > I think that's probably a reasonable optimization, but I don't see why
> > > > anything would require it given its just a cache.
> > > 
> > > I'd append Cut method for the collector to drop all cache after the index
> > 
> > so your talking about more or less seperate improvements we can make to that
> > cache?
> 
> if you agree then it's the right way then separate bug should be fine (if
> you wouldn't want to deal with it here)

ok

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: patch 2

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 725999 [details] [diff] [review]
patch 2

Review of attachment 725999 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/Accessible.cpp
@@ +2648,5 @@
>      return false;
>  
> +  if (aIndex == mChildren.Length()) {
> +    if (!mChildren.AppendElement(aChild))
> +      return false;

nit: extra line would be nice

Comment 12

[[e-mail]]

In regards to the difference between nsTArray's InsertElementAt and AppendElement method, AppendElement will be faster in debug builds but in optimised builds they should theoretically result in the same thing if the compiler is up to the task.

Comment 13

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c5389284d0ad

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5389284d0ad