1477086 - LocalStorageManager.cpp:145:10: warning: local variable 'scope' will be copied despite being returned by name [-Wreturn-std-move]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Daniel Holbert [:dholbert]]

With clang trunk, I get this build warning:
========
dom/storage/LocalStorageManager.cpp:145:10: warning: local variable 'scope' will be copied despite being returned by name [-Wreturn-std-move]
   return scope;
          ^~~~~
======


This happens because scope has type nsAutoCString (and it preferentially uses its stack-allocated buffer), but the function's return type is nsCString (which has no stack-allocated buffer).  So we necessarily have to use a copy-constructor to produce the nsCString return value.

Good news, though - this is just a static function with a few callers (all of which just expect it to return a nsACString of some sort), so we can make it return whatever we want.  Let's just change its return type to nsAutoCString so that we can benefit from return value optimization & reduce copying (in the case where we're using the internal stack-allocated buffer).

Comment 1

[Daniel Holbert [:dholbert]]

This is all about the static function LocalStorageManager::CreateOrigin(), BTW -- I forgot to mention that in comment 0.

I found one callsite where it seems like this change will avoid a string-copy, in LocalStorageManager::GetStorageInternal:
>      StorageDBChild* db = StorageDBChild::Get();
>      if (db) {
>        if (!db->ShouldPreloadOrigin(LocalStorageManager::CreateOrigin(originAttrSuffix, originKey))) {
>          return NS_OK;
https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/dom/storage/LocalStorageManager.cpp#239

In the current state of the world, with CreateOrigin returning nsCString, we have to copy its local nsAutoCString to produce the nsCString return value, which we then use as a temporary variable as an arg to ShouldPreloadOrigin.

But if we make it return nsAutoCString, then this callsite should benefit from return value optimization, and we'll directly pass the returned nsAutoCString into ShouldPreloadOrigin() (which just expects some nsACString&).


Beyond this callsite, I don't think anything will change here -- the other callsites all seem to be in functions that hand off CreateOrigin()'s return value as their own nsCString-typed return value. So those other callsites will still end up doing string-copying to produce a nsCString return value, but they'll just do it at a different level of abstraction.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1477086: Change LocalStorageManager::CreateOrigin()'s return type to nsAutoCString, to benefit from RVO and avoid needless string copying.

Review commit: https://reviewboard.mozilla.org/r/258224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258224/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I was seeing (lots of!) warnings like this one, and while checking if there was already a bug about them I stumbled upon this bug.

mozreview is gone, but luckily the patch is still alive there:
https://hg.mozilla.org/mozreview/gecko/rev/619ee20e40ec

(In reply to Daniel Holbert [:dholbert] from comment #1)
> the other
> callsites all seem to be in functions that hand off CreateOrigin()'s return
> value as their own nsCString-typed return value. So those other callsites
> will still end up doing string-copying to produce a nsCString return value,
> but they'll just do it at a different level of abstraction.

Actually, because you now return `nsAutoCString` from the function, that returned value is temporary and so can be automatically moved-from into an nsCString, potentially avoiding a copy if the string was already too big and therefore lives on the heap.
So I think it's a win-win anyway.

Now, if CreateOrigin is usually bigger than what nsAutoCString can hold, it may be worth using nsCString everywhere instead?

Comment 4

[Daniel Holbert [:dholbert]]

Yeah - the string-copying that I was talking about was for the scenario where we were using a stack-allocated buffer.

Hypothetically if the string is too big for the nsAutoCString stack buffer & it already lives on the heap, then I *think* we already benefit from string-sharing under the hood (I believe our string classes do some form of reference counting on their heap-allocated buffers, to make copying cheap).  So I don't think either the old or new code end up doing any string-copying on the return value in that case, though maybe I'm missing something.

So, given that, I suspect this ends up being a wash for the callsites that force us to convert to nsCString: we still transfer ownership (or still do string-copying-to-the-heap if we're small enough for the stack-allocated buffer), but just do it at a slightly different level.  And for the callsite quoted in comment 1, this is definitely a win.  So over all, a net positive (a "win / neutral" at worst).

asuth, looks like this may've fallen off your radar. Mind reviewing using this link:
 https://hg.mozilla.org/mozreview/gecko/rev/619ee20e40ec
and I can just land via inbound, assuming this looks good to you?

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8993519 [details]
Bug 1477086: Change LocalStorageManager::CreateOrigin()'s return type to nsAutoCString, to benefit from RVO and avoid needless string copying.

Sorry, it was on my review radar but I also had the review of the overhaul of LocalStorage on my radar too, but for various reasons that got more intentionally delayed.  The bit-rot churn here shouldn't be too bad for :janv though, and it would be good to propagate the change on the conflict anyways, so, uh... r=asuth!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4903df8facdd
Change LocalStorageManager::CreateOrigin()'s return type to nsAutoCString, to benefit from RVO and avoid needless string copying. r=asuth

Comment 7

[Daniel Holbert [:dholbert]]

Thanks! Landed.

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4903df8facdd