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.

Comment on attachment 8922133 [details] Bug 1411786 - Use globals for chunk recycling. https://reviewboard.mozilla.org/r/193124/#review198416

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

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.

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?

Comment on attachment 8922137 [details] Bug 1411786 - Make the chunk size and recycle limit constant. https://reviewboard.mozilla.org/r/193132/#review198428

Comment on attachment 8922138 [details] Bug 1411786 - Rename chunk recycling-related globals. https://reviewboard.mozilla.org/r/193134/#review198430

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

Comment on attachment 8922136 [details] Bug 1411786 - More tidying of chunk allocation and recycling code. https://reviewboard.mozilla.org/r/193130/#review198444

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

https://hg.mozilla.org/mozilla-central/rev/00b4a7626a35 https://hg.mozilla.org/mozilla-central/rev/876dc2d16c0e https://hg.mozilla.org/mozilla-central/rev/80292729e86a https://hg.mozilla.org/mozilla-central/rev/4e5c73161240 https://hg.mozilla.org/mozilla-central/rev/fe8c7092d675 https://hg.mozilla.org/mozilla-central/rev/741b8534adf8 https://hg.mozilla.org/mozilla-central/rev/0f749671eb26 https://hg.mozilla.org/mozilla-central/rev/98893371b4c0