Closed Bug 1411786 Opened 7 years ago Closed 7 years ago

Tidy up chunk allocation and recycling

Categories

(Core :: Memory Allocator, enhancement)

Product:
Core
Component:
Memory Allocator
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(8 files)

Bug 1411786 - Don't call chunk_recycle for base allocations.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1411786 - Use globals for chunk recycling.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1411786 - clang-format chunk_{recycle,record,alloc,dealloc}.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1411786 - Rename chunk_{recycle,record,alloc,dealloc} arguments.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1411786 - More tidying of chunk allocation and recycling code.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1411786 - Make the chunk size and recycle limit constant.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1411786 - Rename chunk recycling-related globals.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1411786 - Use mozilla::Atomic for the recycled size count.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
      No description provided.
Summary: Tidy up chunk recycling → Tidy up chunk allocation and recycling
Comment on attachment 8922132 [details]
Bug 1411786 - Don't call chunk_recycle for base allocations.

https://reviewboard.mozilla.org/r/193122/#review198414

::: memory/build/mozjemalloc.cpp:2044
(Diff revision 1)
>  	MOZ_ASSERT((size & chunksize_mask) == 0);
>  	MOZ_ASSERT(alignment != 0);
>  	MOZ_ASSERT((alignment & chunksize_mask) == 0);
>  
> -	if (CAN_RECYCLE(size)) {
> +	// Base allocations can't be fulfilled by recycling because of
> +	// possible deadlock or infinite recursion

Nit: '.' at end of the sentence.
Attachment #8922132 - Flags: review?(n.nethercote) → review+
Comment on attachment 8922133 [details]
Bug 1411786 - Use globals for chunk recycling.

https://reviewboard.mozilla.org/r/193124/#review198416
Attachment #8922133 - Flags: review?(n.nethercote) → review+
Comment on attachment 8922134 [details]
Bug 1411786 - clang-format chunk_{recycle,record,alloc,dealloc}.

https://reviewboard.mozilla.org/r/193126/#review198418

::: memory/build/mozjemalloc.cpp:1979
(Diff revision 1)
>  			 * An additional node is required, but
>  			 * base_node_alloc() can cause a new base chunk to be
>  			 * allocated.  Drop chunks_mtx in order to avoid
>  			 * deadlock, and if node allocation fails, deallocate
>  			 * the result before returning an error.
>  			 */

Fix this comment!

::: memory/build/mozjemalloc.cpp:2104
(Diff revision 1)
> -        /*
> +  /*
>  	 * Allocate a node before acquiring chunks_mtx even though it might not
>  	 * be needed, because base_node_alloc() may cause a new base chunk to
>  	 * be allocated, which could cause deadlock if chunks_mtx were already
>  	 * held.
>  	 */

Fix comment.

::: memory/build/mozjemalloc.cpp:2118
(Diff revision 1)
> -        if (node && node->addr == key.addr) {
> +  if (node && node->addr == key.addr) {
> -                /*
> +    /*
>  		 * Coalesce chunk with the following address range.  This does
>  		 * not change the position within chunks_ad, so only
>  		 * remove/insert from/into chunks_szad.
>  		 */

Fix comment.

::: memory/build/mozjemalloc.cpp:2134
(Diff revision 1)
> -                        /*
> +      /*
>  			 * base_node_alloc() failed, which is an exceedingly
>  			 * unlikely failure.  Leak chunk; its pages have
>  			 * already been purged, so this is only a virtual
>  			 * memory leak.
>  			 */

Fix comment.

::: memory/build/mozjemalloc.cpp:2153
(Diff revision 1)
> -	    chunk) {
> -                /*
> +    /*
>  		 * Coalesce chunk with the previous address range.  This does
>  		 * not change the position within chunks_ad, so only
>  		 * remove/insert node from/into chunks_szad.
>  		 */

ditto

::: memory/build/mozjemalloc.cpp:2175
(Diff revision 1)
>  label_return:
> -        chunks_mtx.Unlock();
> +  chunks_mtx.Unlock();
> -        /*
> +  /*
>  	 * Deallocate xnode and/or xprev after unlocking chunks_mtx in order to
>  	 * avoid potential deadlock.
>  	 */

ditto
Attachment #8922134 - Flags: review?(n.nethercote) → review+
Comment on attachment 8922135 [details]
Bug 1411786 - Rename chunk_{recycle,record,alloc,dealloc} arguments.

https://reviewboard.mozilla.org/r/193128/#review198420

Oh, you fixed the comment indendations here.

::: memory/build/mozjemalloc.cpp:1175
(Diff revision 1)
>  /******************************************************************************/
>  /*
>   * Begin forward declarations.
>   */
>  
> -static void	*chunk_alloc(size_t size, size_t alignment, bool base, bool *zeroed=nullptr);
> +static void* chunk_alloc(size_t aSize, size_t aAlignment, bool aBase, bool* aZeroed=nullptr);

Nit: spaces around '='.

::: memory/build/mozjemalloc.cpp:2053
(Diff revision 1)
>        goto RETURN;
> -  }
> +    }
> -  ret = chunk_alloc_mmap(size, alignment);
> -  if (zeroed)
> -    *zeroed = true;
> +  }
> +  ret = chunk_alloc_mmap(aSize, aAlignment);
> +  if (aZeroed)
> +    *aZeroed = true;

Braces.
Attachment #8922135 - Flags: review?(n.nethercote) → review+
Comment on attachment 8922136 [details]
Bug 1411786 - More tidying of chunk allocation and recycling code.

https://reviewboard.mozilla.org/r/193130/#review198424

::: memory/build/mozjemalloc.cpp:2052
(Diff revision 1)
>    // Base allocations can't be fulfilled by recycling because of
>    // possible deadlock or infinite recursion
>    if (CAN_RECYCLE(aSize) && !aBase) {
>      ret = chunk_recycle(aSize, aAlignment, aZeroed);
>      if (ret) {
> -      goto RETURN;
> +      return ret;

If we reach this |goto| then `ret && aBase == false` is true, which means when we jump to RETURN we execute the `if (!gChunkRTree.Set(ret, ret))` block. By changing to return early here I think you've changed behaviour?
Attachment #8922136 - Flags: review?(n.nethercote) → review-
Comment on attachment 8922137 [details]
Bug 1411786 - Make the chunk size and recycle limit constant.

https://reviewboard.mozilla.org/r/193132/#review198428
Attachment #8922137 - Flags: review?(n.nethercote) → review+
Comment on attachment 8922138 [details]
Bug 1411786 - Rename chunk recycling-related globals.

https://reviewboard.mozilla.org/r/193134/#review198430
Attachment #8922138 - Flags: review?(n.nethercote) → review+
Comment on attachment 8922134 [details]
Bug 1411786 - clang-format chunk_{recycle,record,alloc,dealloc}.

https://reviewboard.mozilla.org/r/193126/#review198418

> Fix this comment!

FWIW, I filed bug #1411807 about clang-format not handling this.
(In reply to Nicholas Nethercote [:njn] from comment #13)
> Comment on attachment 8922136 [details]
> Bug 1411786 - More tidying of chunk allocation and recycling code.
> 
> https://reviewboard.mozilla.org/r/193130/#review198424
> 
> ::: memory/build/mozjemalloc.cpp:2052
> (Diff revision 1)
> >    // Base allocations can't be fulfilled by recycling because of
> >    // possible deadlock or infinite recursion
> >    if (CAN_RECYCLE(aSize) && !aBase) {
> >      ret = chunk_recycle(aSize, aAlignment, aZeroed);
> >      if (ret) {
> > -      goto RETURN;
> > +      return ret;
> 
> If we reach this |goto| then `ret && aBase == false` is true, which means
> when we jump to RETURN we execute the `if (!gChunkRTree.Set(ret, ret))`
> block. By changing to return early here I think you've changed behaviour?

Doh, indeed, and that's the cause of the orange cpp unit tests on try.
Comment on attachment 8922139 [details]
Bug 1411786 - Use mozilla::Atomic for the recycled size count.

https://reviewboard.mozilla.org/r/193136/#review198440
Attachment #8922139 - Flags: review?(n.nethercote) → review+
Comment on attachment 8922136 [details]
Bug 1411786 - More tidying of chunk allocation and recycling code.

https://reviewboard.mozilla.org/r/193130/#review198444
Attachment #8922136 - Flags: review?(n.nethercote) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/00b4a7626a35
Don't call chunk_recycle for base allocations. r=njn
https://hg.mozilla.org/integration/autoland/rev/876dc2d16c0e
Use globals for chunk recycling. r=njn
https://hg.mozilla.org/integration/autoland/rev/80292729e86a
clang-format chunk_{recycle,record,alloc,dealloc}. r=njn
https://hg.mozilla.org/integration/autoland/rev/4e5c73161240
Rename chunk_{recycle,record,alloc,dealloc} arguments. r=njn
https://hg.mozilla.org/integration/autoland/rev/fe8c7092d675
More tidying of chunk allocation and recycling code. r=njn
https://hg.mozilla.org/integration/autoland/rev/741b8534adf8
Make the chunk size and recycle limit constant. r=njn
https://hg.mozilla.org/integration/autoland/rev/0f749671eb26
Rename chunk recycling-related globals. r=njn
https://hg.mozilla.org/integration/autoland/rev/98893371b4c0
Use mozilla::Atomic for the recycled size count. r=njn
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: