1439047 - Improve StartupCache::PutBuffer

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Eric Rahm [:erahm]]

`StartupCache::PutBuffer` can be improved in a couple of ways:

1) We pass in a buffer, alloc a UniquePtr, and copy the buffer into it. Each caller is actually passing in a temporary buffer from a smart container (UniquePtr or Vector). Instead we should just hand over ownership and avoid the copy.

2) We check if an entry is in the hashtable and then later we add it. We should use LookupForAdd to avoid the multiple lookups.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Make StartupCache::PutBuffer take ownership of the buffer

This avoids a redundant alloc and copy in `PutBuffer`. All existing callers
were destroying the passed in buffer after the call.

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Cleanup StartupCache::PutBuffer hashtable usage

This switches over to using `LookupForAdd` which allows us to avoid a second
lookup when adding the entry. Addtionally `nsDependentCString` is used to
avoid copying the id string when looking up the entry.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8951818 [details] [diff] [review]
Part 1: Make StartupCache::PutBuffer take ownership of the buffer

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

I like this.  If the below problems aren't actually problems in automation, we should probably land this.

::: js/xpconnect/loader/mozJSLoaderUtils.cpp
@@ +63,5 @@
>      if (size > UINT32_MAX)
>          return NS_ERROR_FAILURE;
> +
> +    // Move the vector buffer into a unique pointer buffer.
> +    UniquePtr<char[]> buf(reinterpret_cast<char*>(buffer.extractOrCopyRawBuffer()));

Same dodginess with mismatched allocator/deallocator here.

::: startupcache/test/TestStartupCache.cpp
@@ +96,4 @@
>    UniquePtr<char[]> outbuf;
>    uint32_t len;
>  
> +  rv = sc->PutBuffer(id, UniquePtr<char[]>(strdup(buf)), strlen(buf) + 1);

This is a little dodgy, since we'll allocate memory with malloc (presumably), but we'll free it with delete[] via the UniquePtr.  I think this works out OK for most cases, but it's possible ASan or Valgrind will complain about this.

@@ +122,4 @@
>    StartupCache* sc = StartupCache::GetSingleton();
>    ASSERT_TRUE(sc);
>  
> +  rv = sc->PutBuffer(id, UniquePtr<char[]>(strdup(buf)), strlen(buf) + 1);

Sam here.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8951819 [details] [diff] [review]
Part 2: Cleanup StartupCache::PutBuffer hashtable usage

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

Thanks!

Comment 5

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8951818 [details] [diff] [review]
> Part 1: Make StartupCache::PutBuffer take ownership of the buffer
> 
> Review of attachment 8951818 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like this.  If the below problems aren't actually problems in automation,
> we should probably land this.
> 
> ::: js/xpconnect/loader/mozJSLoaderUtils.cpp
> @@ +63,5 @@
> >      if (size > UINT32_MAX)
> >          return NS_ERROR_FAILURE;
> > +
> > +    // Move the vector buffer into a unique pointer buffer.
> > +    UniquePtr<char[]> buf(reinterpret_cast<char*>(buffer.extractOrCopyRawBuffer()));
> 
> Same dodginess with mismatched allocator/deallocator here.
> 
> ::: startupcache/test/TestStartupCache.cpp
> @@ +96,4 @@
> >    UniquePtr<char[]> outbuf;
> >    uint32_t len;
> >  
> > +  rv = sc->PutBuffer(id, UniquePtr<char[]>(strdup(buf)), strlen(buf) + 1);
> 
> This is a little dodgy, since we'll allocate memory with malloc
> (presumably), but we'll free it with delete[] via the UniquePtr.  I think
> this works out OK for most cases, but it's possible ASan or Valgrind will
> complain about this.
> 
> @@ +122,4 @@
> >    StartupCache* sc = StartupCache::GetSingleton();
> >    ASSERT_TRUE(sc);
> >  
> > +  rv = sc->PutBuffer(id, UniquePtr<char[]>(strdup(buf)), strlen(buf) + 1);
> 
> Sam here.

ASAN/V don't seem to care; we're already doing something similar with GetBuffer in `ReadCachedScript` [1].

[1] https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/js/xpconnect/loader/mozJSLoaderUtils.cpp#26-33

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0cedfb4bd8
Part 1: Make StartupCache::PutBuffer take ownership of the buffer. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/718961d941d4
Part 2: Cleanup StartupCache::PutBuffer hashtable usage. r=froydnj

Comment 7

[Narcis Beleuzu [:NarcisB]]

Backed out for Android build bustages

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=164039562&repo=mozilla-inbound&lineNumber=20828
Push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0b9ff594f7711778efe033de8de9dda4bed451f5

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2511fb2fd5cf
Backed out 2 changesets for Android build bustages. CLOSED TREE

Comment 9

[Eric Rahm [:erahm]]

(In reply to Narcis Beleuzu [:NarcisB] from comment #7)
> Backed out for Android build bustages
> 
> Log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=164039562&repo=mozilla-
> inbound&lineNumber=20828
> Push:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=0b9ff594f7711778efe033de8de9dda4bed451f5

Looks like there was some Android only usage of PutBuffer that I missed. Think I have a fix, doing a try run on android now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=553d932152d6530dfc3c5fac68c417d5644a3412

Comment 10

[Eric Rahm [:erahm]]

...and we're good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0117739258f469a9713f9c306480da4c4dd4c9b5

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4b5c48ed52
Part 1: Make StartupCache::PutBuffer take ownership of the buffer. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dc8f7e2eee4
Part 2: Cleanup StartupCache::PutBuffer hashtable usage. r=froydnj

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1d4b5c48ed52
https://hg.mozilla.org/mozilla-central/rev/7dc8f7e2eee4