927078 - Don't copy string data when sending messages using message manager

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

We could just improve nsDependentString a bit and use it to pass data
to IPC layer.
Patch coming.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

nsDependentString is a bit error prone, but the patch doesn't make it any
worse. Requirement to pass null-terminated string to it is left to the caller anyway.


(And need to fix the name AssertValidDepedentString at some point, but not in this bug)

Comment 2

[Benjamin Smedberg]

Comment on attachment 817431 [details] [diff] [review]
patch

The constructor seems like a footgun for future generations.

Could we make the callsites intentionally uglier so that instead of nsDependentString(aMessage) its nsDependentString(aMessage.BeginReading(), aMessage.Length()) ?

But also more generally, how do you know that aMessage (in ChildProcessMessageManagerCallback::DoSendBlockingMessage) actually is null-terminated? I don't see any external guarantees of that.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> The constructor seems like a footgun for future generations.
Not more than our current nsDependentString ctors

> 
> But also more generally, how do you know that aMessage (in
> ChildProcessMessageManagerCallback::DoSendBlockingMessage) actually is
> null-terminated? I don't see any external guarantees of that.
Well, we get null-terminated strings from JS. and MM is JS-only API.

I could do something uglier but safer - basically check whether the string is null terminated and only
then use DependentString and otherwise copy data.

Comment 4

[Dietrich Ayala (:dietrich)]

Hi Olli, we're looking for any little wins we can get for tarako. Any time to finish this up?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

/me doesn't know what tarako is.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

But sure, I could take a look at this again.

Comment 7

[Gabriele Svelto [:gsvelto]]

(In reply to Olli Pettay [:smaug] from comment #5)
> /me doesn't know what tarako is.

It's a FxOS device with 128MiB of RAM, so it's a platform where every bit counts (literally).

See the wiki page for more info https://wiki.mozilla.org/FirefoxOS/Tarako

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: maybedependentstring.diff

Something like this then. Safer, and removes still lots of extra copying.

https://tbpl.mozilla.org/?tree=Try&rev=8bfa74513f7f

Comment 9

[Benjamin Smedberg]

Comment on attachment 8375896 [details] [diff] [review]
maybedependentstring.diff

r=me but I'd like dbaron to review the API as well.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

How is this different from the existing PromiseFlat[C]String functions, which use the nsPromiseFlat[C]String class?

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(We should probably rename those from PromiseFlat to PromiseTerminated, given that we no longer have multi-fragment strings.  Flat is the name we used meaning both single-fragment and terminated.)

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8375896 [details] [diff] [review]
maybedependentstring.diff

I don't see any difference between this and PromiseFlatString; I think you should just use that.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Crap, we have two kinds of PromiseFlatStrings. One which is useful, and one in glue which is just
typedef from nsString. And I was aware of only the latter. I didn't know PromiseFlatStrings had this kind of odd side effect.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(We should not have inconsistent APIs. glue exposing totally different API behavior with the same name is horrible.)

Comment 15

[Benjamin Smedberg]

The glue version does in fact promise a flat string. It just always performs a share or a copy instead of sometimes doing a dependent string, because the glue API isn't aware of the flags for that. And it was necessary to add this to the glue to compile mailnews in some configurations.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: promiseFlatString

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fd55911a3c9

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fd55911a3c9

Comment 19

[Joe Cheng [:jcheng] (please needinfo)]

Hi Olli, I wonder if we know how much memory saving this patch can provide? trying to evaluate if this is needed for Tarako project (128MB RAM Firefox OS project). thanks

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

This is more performance win than memory saving.
Well, it reduces memory usage too, but that just a short amount memory during a very short period.

Comment 21

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/a403879a3ce2