1223808 - replace nsAutoArrayPtr<T> with UniquePtr<T[]> in netwerk/

Status: RESOLVED FIXED

Description

[Nathan Froyd [:froydnj]]

Comment 1

[Nathan Froyd [:froydnj]]

Created attachment 8686061 [details] [diff] [review]
part 1 - optimize creating a WebSocketFrame with a payload

This function:

- Allocates a block of memory;
- Copies things into that block of memory;
- Allocates another block of memory inside nsAutoCString;
- Copies into our string from the block of memory we just allocated.

Clearly we can avoid a memory allocation and some copying by just
allocating the amount of data we need in the string first, and copying
things into the string.

Comment 2

[Nathan Froyd [:froydnj]]

Created attachment 8686062 [details] [diff] [review]
part 2 - don't dynamically allocate HandleHashKey::mHash

Converting HandleHashKey's use of nsAutoArrayPtr is a little difficult,
because:

  MakeUnique<SHA1Sum::Hash>();

doesn't work, and trying to do something like:

  mHash.reset(reinterpret_cast<SHA1Sum::Hash*>(new uint8_t[SHA1Sum::kHashSize]));

is just ugly.  Since we're always allocating it anyway, let's just store
it directly in HandleHashKey and avoid the dynamic allocation in the
first place.

Comment 3

[Nathan Froyd [:froydnj]]

Created attachment 8686063 [details] [diff] [review]
part 3 - replace nsAutoArrayPtr<T> with UniquePtr<T[]> in netwerk/

Comment 4

[Michal Novotny [:michal]]

Comment on attachment 8686062 [details] [diff] [review]
part 2 - don't dynamically allocate HandleHashKey::mHash

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

The reason why the hash is dynamically allocated is that in CacheFileHandle we need a pointer to the hash that won't change. If the hash is stored in HandleHashKey, then CacheFileHandle::mHash becomes invalid sooner or later, right?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Michal Novotny (:michal) from comment #4)
> The reason why the hash is dynamically allocated is that in CacheFileHandle
> we need a pointer to the hash that won't change. If the hash is stored in
> HandleHashKey, then CacheFileHandle::mHash becomes invalid sooner or later,
> right?

Ah, yes.  This explains the crashes I was getting shortly after startup, sometime after I posted this patch. :)  I guess the alternative is some liberal use of reinterpret_cast.

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8686063 [details] [diff] [review]
part 3 - replace nsAutoArrayPtr<T> with UniquePtr<T[]> in netwerk/

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

thanks.. that will get picked up as an idiom

Comment 7

[Nathan Froyd [:froydnj]]

Created attachment 8689187 [details] [diff] [review]
part 2 - use UniquePtr<uint8_t[]> instead of nsAutoArrayPtr<SHA1Sum::Hash> in HandleHashKey

Here's a patch that seems to work better.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/689d9f449560
https://hg.mozilla.org/integration/mozilla-inbound/rev/37997f6e4349
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc847f227d9a

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/689d9f449560
https://hg.mozilla.org/mozilla-central/rev/37997f6e4349
https://hg.mozilla.org/mozilla-central/rev/cc847f227d9a