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

RESOLVED FIXED in Firefox 45

Status

()

Core
Networking
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
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.
Attachment #8686061 - Flags: review?(mcmanus)
(Assignee)

Comment 2

3 years ago
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.
Attachment #8686062 - Flags: review?(michal.novotny)
(Assignee)

Comment 3

3 years ago
Created attachment 8686063 [details] [diff] [review]
part 3 - replace nsAutoArrayPtr<T> with UniquePtr<T[]> in netwerk/
Attachment #8686063 - Flags: review?(mcmanus)
Attachment #8686061 - Flags: review?(mcmanus) → review+
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?
Attachment #8686062 - Flags: review?(michal.novotny) → review-
(Assignee)

Comment 5

3 years ago
(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 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
Attachment #8686063 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 7

3 years ago
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.
Attachment #8689187 - Flags: review?(michal.novotny)
(Assignee)

Updated

3 years ago
Attachment #8686062 - Attachment is obsolete: true
Attachment #8689187 - Flags: review?(michal.novotny) → review+
(Assignee)

Updated

3 years ago
Blocks: 1229985

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/689d9f449560
https://hg.mozilla.org/mozilla-central/rev/37997f6e4349
https://hg.mozilla.org/mozilla-central/rev/cc847f227d9a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.