Closed Bug 769816 Opened 9 years ago Closed 9 years ago

Send newline-delimited request bodies

Categories

(Cloud Services :: Firefox: Common, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla16

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(1 file)

This is needed to enable easier accounting of the current outgoing request size so we can batch requests.
Attachment #638014 - Flags: review?(rnewman)
Comment on attachment 638014 [details] [diff] [review]
Send newline-delimited requests, v1

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

::: services/common/storageservice.js
@@ +1114,5 @@
>  
> +    let line = JSON.stringify(bso).replace("\n", "\u000a");
> +
> +    // The 1 is for the newline between records. We don't care we might be
> +    // off by 1.

How do you mean? Because "\n" could be one or two characters depending on platform?

If so, why don't you use

  this._size += line.length + "\n".length;

or equivalent?

Otherwise, please clarify comment...

@@ +1121,4 @@
>    },
>  
>    _onDispatch: function _onDispatch() {
> +    this._data = this._lines.join("\n");

Perhaps now is a good time to think about exposing the inner stream from RESTRequest. We're allocating a huge string (plus its components!) every time we do an upload.

(Yes, we allocated a huge string before. We'd hold onto the individual BSOs, and compute a single huge JSON array string. Now we hold onto the string representation of each BSO, then the huge string of the joined lines.)

In our case we could perhaps use an nsIMultiplexInputStream to combine an array of nsIStringInputStreams that use shareData() rather than setData(), which will avoid allocating a new string altogether, or implement a specialized nsIStringsInputStream…

Please consider filing a follow-up here.
Attachment #638014 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #1)
> Comment on attachment 638014 [details] [diff] [review]
> Send newline-delimited requests, v1
> 
> Review of attachment 638014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/common/storageservice.js
> @@ +1114,5 @@
> >  
> > +    let line = JSON.stringify(bso).replace("\n", "\u000a");
> > +
> > +    // The 1 is for the newline between records. We don't care we might be
> > +    // off by 1.
> 
> How do you mean? Because "\n" could be one or two characters depending on
> platform?

The off by 1 case is when there is a single record. There is no trailing newline in the request body, yet I count it in the size.

> 
> If so, why don't you use
> 
>   this._size += line.length + "\n".length;

Wut? How is "\n" not 0x0A and not 1 byte?

> @@ +1121,4 @@
> >    },
> >  
> >    _onDispatch: function _onDispatch() {
> > +    this._data = this._lines.join("\n");
> 
> Perhaps now is a good time to think about exposing the inner stream from
> RESTRequest. We're allocating a huge string (plus its components!) every
> time we do an upload.

Yes, we should consider streaming outgoing requests. Pretty sure we already have a bug on file. That will also tackle our potential memory issue.
(In reply to Gregory Szorc [:gps] from comment #2)

> Wut? How is "\n" not 0x0A and not 1 byte?

Conditional question based on above. Was trying to figure out what you mean by off by one!

> Yes, we should consider streaming outgoing requests. Pretty sure we already
> have a bug on file. That will also tackle our potential memory issue.

Could you find it and cross-link, or file if not?
Bug 693812 is related.
https://hg.mozilla.org/mozilla-central/rev/f6a5b82eb76b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.