be smarter about constructing string input streams

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments)

We can have, at the very least, a string stream constructor that accepts rvalue
references, so we can avoid needless copying.
This method is more efficient when we know we're not going to need the
string afterwards, and should cut down on intermediate allocations.

I figured updating various callsites to use this new function was reasonable.
Generally, the only tweaks necessary were to record the lengths of the
going-to-be-Move()'d strings.  If you don't feel comfortable reviewing those
changes, let me know, and I can try to find someone else.
Attachment #8966736 - Flags: review?(amarchesini)
The XXX comment here wants to give up the string data when we create the
outgoing stream.  Giving up the string data is legitimate, because
GetEncodedSubmission is the last operation to be called on this object;
mQueryString is effectively dead after this function returns.
Accordingly, we can Move() mQueryString into the outgoing stream for a
nice efficiency boost.

This change seemed to be a little more involved than the mostly mechanical
changes in the previous patch, and require some actual knowledge about the code
to demonstrate that the result is correct.  So I thought this was worth
separating out into its own patch.
Attachment #8966737 - Flags: review?(amarchesini)
We could have used the new NS_NewCStringInputStream overload here, but
it seemed nicer to directly transfer ownership into the newly-created
stream.  If we're going to be more efficient here, we might as well go
as far as when can without making the code too ugly.
Attachment #8966738 - Flags: review?(amarchesini)
Attachment #8966736 - Flags: review?(amarchesini) → review+
Comment on attachment 8966737 [details] [diff] [review]
part 2 - use new string input stream constructor in FSURLEncoded

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

This is correct. GetEncodedSubmission() is the last method to be called on FSURLEncoded, or FSMultipartFormData and so on.
Unfortunately there is nothing enforcing it.
Attachment #8966737 - Flags: review?(amarchesini) → review+
Attachment #8966738 - Flags: review?(amarchesini) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c19f1a86b85e
part 1 - add a string input stream constructor that accepts move references; r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/10ec47ed7a95
part 2 - use new string input stream constructor in FSURLEncoded; r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca38b3ded716
part 3 - be smarter about input stream semantics in DataTransfer; r=baku
You need to log in before you can comment on or make changes to this bug.