769816 - Send newline-delimited request bodies

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Send newline-delimited requests, v1

This is needed to enable easier accounting of the current outgoing request size so we can batch requests.

Comment 1

[Richard Newman [: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.

Comment 2

[Gregory Szorc [:gps]]

(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.

Comment 3

[Richard Newman [:rnewman]]

(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?

Comment 4

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/f6a5b82eb76b

Comment 5

[Gregory Szorc [:gps]]

Bug 693812 is related.

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/f6a5b82eb76b