233238 - nsContainerEnumerator::HasMoreElements shouldn't recompute upper bound every time

Status: RESOLVED INCOMPLETE

(Core Graveyard :: RDF, defect)

Description

[Chase Tingley]

I finally got jprof to work, so I decided to have a look at bug 227346 by
profiling opening the download manager with a very large (10k entries, about
7.2M) downloads.rdf file.  The first thing that popped out at me was this:

105463   3     1987 ContainerEnumeratorImpl::HasMoreElements(int*)

It turns out HasMoreElements() is doing a query for the value of rdf:nextVal
every time, in an attempt to figure out how far to iterate:
http://lxr.mozilla.org/seamonkey/source/rdf/base/src/nsContainerEnumerator.cpp#171

I'm not sure this makes sense.  Furthermore, if we calculate the max once during
Init(), rather than every time through the loop, we can "save a buck or two".

Comment 1

[Chase Tingley]

Attachment: move nextval computation to Init()

The patch moves the nextval computation into Init() and saves the result.  Note
that if the size of the container were somehow reduced during the enumeration,
HasMoreElements() would return PR_FALSE at the right time, when the
GetTargets() call for the next ordinal comes up empty..

In an opt build, this shortens the average time needed to open the download
manager with a 10k entry downloads.rdf file from 1:56 to 1:43.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Note that if the size of the container were somehow reduced during the
> enumeration,

What if it were increased?

Comment 3

[Chase Tingley]

Yeah, that was the basis for my original question about the desired semantics --
this would only give you the first n elements if it were expanded to n+m while
iterating.

It's not clear to me what sort of guarantees RDF makes with its enumerators --
for instance, InMemoryDataSource::GetAllResources() generally has the same
problem, where resources that are added after the enumerator is initialized
aren't included.

Comment 4

[Chase Tingley]

Another alternative would be to never check nextVal at all -- just keep walking
through higher and higher ordinal properties until there are no more targets.

Comment 5

[Benjamin Smedberg]

Comment on attachment 140713 [details] [diff] [review]
move nextval computation to Init()

I've been thinking about this, and I don't think this is a good solution. We
should be taking "snapshot" enumerations, instead of dynamic enumerations. So
the correct answer would be to walk the list once, when the enumerator is
created, and keep all the values in a comarray. Then just call
NS_NewArrayEnumerator on it when we're done. The only real problem I can
envision with this is if callers don't *need* to get all the arcs, but just the
first few.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The whole point of using enumerators is so you don't have to have all the
thingies in memory at once...

Comment 7

[Axel Hecht]

This is actually not only an implementation issue, but an API issue.

The API doesn't say wether it is working on a snapshot of the container or if
the container iteration is live.

Adding comment on that to bug 216788.