Closed Bug 1439047 Opened 2 years ago Closed 2 years ago

Improve StartupCache::PutBuffer

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: erahm, Assigned: erahm)

Details

Attachments

(2 files)

`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.
This avoids a redundant alloc and copy in `PutBuffer`. All existing callers
were destroying the passed in buffer after the call.
Attachment #8951818 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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.
Attachment #8951819 - Flags: review?(nfroyd)
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.
Attachment #8951818 - Flags: review?(nfroyd) → review+
Comment on attachment 8951819 [details] [diff] [review]
Part 2: Cleanup StartupCache::PutBuffer hashtable usage

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

Thanks!
Attachment #8951819 - Flags: review?(nfroyd) → review+
(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
Pushed by erahm@mozilla.com:
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
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2511fb2fd5cf
Backed out 2 changesets for Android build bustages. CLOSED TREE
(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
Flags: needinfo?(erahm)
Pushed by erahm@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/1d4b5c48ed52
https://hg.mozilla.org/mozilla-central/rev/7dc8f7e2eee4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.