1197400 - Make nsBaseHashtable::ConstIter mean something

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Fairly straightforward, except for the continued presence of Data() on ConstIter.  We have to keep it for cases where UserDataType is a nsTArray since UserData() returns by value, not by reference, and we don't want to copy construct a new nsTArray.

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 8651268 [details] [diff] [review]
Patch

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

I don't see the point of this. And you haven't done it for PLDHashTable or nsTHashtable, which reduces consistency.

::: xpcom/glue/nsBaseHashtable.h
@@ +263,5 @@
> +      return Base::Done();
> +    }
> +    void Next() {
> +      Base::Next();
> +    }

Why not just let the inherited versions of these functions be used? These versions aren't any different, AFAICT.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The point is to prevent you from calling Remove() on the value returned from hashtable.ConstIter().

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8651268 [details] [diff] [review]
Patch

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

This UserData/Data thing is just stupid.  So hard to tell at the callsite whether something is copying or not.  I think I caught all the ones below, and most of the ones with |auto| could profit from |auto*|, just to make sure.

I would hope that we could fix some of these to not copy...even if it is just memory reporting code.

::: dom/base/nsDOMAttributeMap.cpp
@@ +561,5 @@
>    size_t n = aMallocSizeOf(this);
>  
>    n += mAttributeCache.ShallowSizeOfExcludingThis(aMallocSizeOf);
>    for (auto iter = mAttributeCache.ConstIter(); !iter.Done(); iter.Next()) {
> +    n += aMallocSizeOf(iter.UserData());

I know this is memory reporting code, but this is going to unnecessarily refcount, isn't it?

::: dom/indexedDB/ActorsParent.cpp
@@ +9645,5 @@
>                   "DatabaseConnection::UpdateRefcountFunction::DidCommit",
>                   js::ProfileEntry::Category::STORAGE);
>  
>    for (auto iter = mFileInfoEntries.ConstIter(); !iter.Done(); iter.Next()) {
> +    auto value = iter.UserData();

Making some of these |auto*|, when applicable, would be super helpful in knowing that we're not unnecessarily copying...

::: layout/style/CSSVariableDeclarations.cpp
@@ +211,5 @@
>    size_t n = aMallocSizeOf(this);
>    n += mVariables.ShallowSizeOfExcludingThis(aMallocSizeOf);
>    for (auto iter = mVariables.ConstIter(); !iter.Done(); iter.Next()) {
>      n += iter.Key().SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +    n += iter.UserData().SizeOfExcludingThisIfUnshared(aMallocSizeOf);

Doesn't this copy now, since UserData returns nsString?

::: layout/style/Loader.cpp
@@ +2601,5 @@
>           iter.Next()) {
>        // If aSheet has a parent, then its parent will report it so we don't
>        // have to worry about it here. Likewise, if aSheet has an owning node,
>        // then the document that node is in will report it.
> +      const nsRefPtr<CSSStyleSheet>& aSheet = iter.UserData();

Doesn't this copy now, since UserData returns nsRefPtr<CSSStyleSheet>?

::: netwerk/cache2/CacheFile.cpp
@@ +1994,4 @@
>    }
>    n += mCachedChunks.ShallowSizeOfExcludingThis(mallocSizeOf);
>    for (auto iter = mCachedChunks.ConstIter(); !iter.Done(); iter.Next()) {
> +      n += iter.UserData()->SizeOfIncludingThis(mallocSizeOf);

Don't both of these copy now, since UserData in these cases is nsRefPtr<CacheFileChunk>?

::: xpcom/glue/nsBaseHashtable.h
@@ +263,5 @@
> +      return Base::Done();
> +    }
> +    void Next() {
> +      Base::Next();
> +    }

I think you can say:

using Base::Done;
using Base::Next;

instead.

Comment 5

[Nathan Froyd [:froydnj]]

Fixing up the consistency thing would be good; Kyle, can you file followup bugs on that?

It seems like people prefer the STL semantics of const-ness when it comes to iterators.

Comment 6

[Nicholas Nethercote [inactive]]

Fixing this in PLDHashTable first, then nsTHashtable, then nsBaseHashtable, feels like the right way to do it.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Pretty clear that I'm not actually going to finish this.