298613 - there should be a way to quickly insert multiple items into an RDF container

Status: RESOLVED WONTFIX

(Core Graveyard :: RDF, defect)

Description

[Jan Darmochwal]

Inserting multiple items into an RDF container with
RDFContainerImpl::InsertElementAt() is very slow, because the container elements
have to be renumbered for each item inserted. Things could be much faster if the
renumbering was done for all the new items before inserting them.
One way to do this might be to make RDFContainerImpl::Renumber() scriptable via
nsIRDFContainer and then call Renumber() before doing the InsertElementAt()s
with aRenumber set to false.
A problem with this approach could be that aborting the insertion before all the
elements have been inserted might leave a "gap" in the container.

Comment 1

[Jan Darmochwal]

Attachment: make RDFContainerImpl::Renumber() scriptable via nsIRDFContainer

Comment 2

[Axel Hecht]

Comment on attachment 187145 [details] [diff] [review]
make RDFContainerImpl::Renumber() scriptable via nsIRDFContainer

This spreads brittle code over the platform, I favour a insertElementsAt, with
some [array, size_is()] goodness. This may be usefull to web apps, and I don't
feel like making them jump the nsIArray loop.

This patch messes with cvs blame more than necessary, too.

Comment 3

[Axel Hecht]

shaver may want to verify that I'm not smoking crack in #2 about the array foo.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I can't speak reliably to your crack habits, but

void insertElementsAt([array,size_is(eltCount)] in nsIWhatever elements,
                      in PRUint32 eltCount, in PRUint32 index);

seems right to me.

Comment 5

[Jan Darmochwal]

Attachment: new methods InsertElementsAt() and RemoveElementsAt()

Comment 6

[Axel Hecht]

Comment on attachment 187270 [details] [diff] [review]
new methods InsertElementsAt() and RemoveElementsAt()

>Index: rdf/base/src/nsRDFContainer.cpp

>@@ -298,97 +298,122 @@ 
<...>
> NS_IMETHODIMP
> RDFContainerImpl::InsertElementAt(nsIRDFNode *aElement, PRInt32 aIndex, PRBool aRenumber)
> {
>+    return InsertElementsAt(&aElement, 1, aIndex, aRenumber);
>+}

Make this crash safe for aElement == nsnull, too.

<...>

> NS_IMETHODIMP
> RDFContainerImpl::RemoveElementAt(PRInt32 aIndex, PRBool aRenumber, nsIRDFNode** _retval)
> {
>+    return RemoveElementsAt(aIndex, 1, aRenumber, &_retval);
>+}

You're assigning a one element array allocated to a reference here, I expect
this to crash.

<...>

Comment 7

[Jan Darmochwal]

Attachment: new methods InsertElementsAt() and RemoveElementsAt()

next try

> >+	return InsertElementsAt(&aElement, 1, aIndex, aRenumber);
> 
> Make this crash safe for aElement == nsnull, too.

InsertElementsAt() checks if it's nsnull.

Comment 8

[Axel Hecht]

Comment on attachment 188526 [details] [diff] [review]
new methods InsertElementsAt() and RemoveElementsAt()


> NS_IMETHODIMP
> RDFContainerImpl::InsertElementAt(nsIRDFNode *aElement, PRInt32 aIndex, PRBool aRenumber)
> {
>+    return InsertElementsAt(&aElement, 1, aIndex, aRenumber);
			      ^^^
this crashes.



> NS_IMETHODIMP
> RDFContainerImpl::RemoveElementAt(PRInt32 aIndex, PRBool aRenumber, nsIRDFNode** _retval)
> {
>+    PRUint32 count;
>+    nsIRDFNode **elements;
>+    nsresult rv = RemoveElementsAt(aIndex, 1, aRenumber, &count, &elements);
>+    *_retval = NS_SUCCEEDED(rv) ? elements[0] : nsnull;
>+    return rv;
>+}

This leaks elements.

Comment 9

[Jan Darmochwal]

Attachment: new methods InsertElementsAt() and RemoveElementsAt()

time for some more tries

Comment 10

[Axel Hecht]

Comment on attachment 210346 [details] [diff] [review]
new methods InsertElementsAt() and RemoveElementsAt()

Sorry for the lag again, took me a while to actually look at the review queue of this account.

r-, the [array] foo is still broken, as that needs to allocate the array itself.

This is a low cost r-, too, given that bookmarks are moving from RDF over to sqlite places, I'm not convinced that we need to have this.

Comment 11

[Phoenix]

With moving from RDF to Places, is this bug still actual?

Comment 12

[Jan Darmochwal]

(In reply to Phoenix from comment #11)
> With moving from RDF to Places, is this bug still actual?

It's probably OK to resolve this bug as WONTFIX. There does not seem to be much need for fast insertions in RDF containers anymore.