Closed Bug 1401099 Opened 2 years ago Closed 2 years ago

Some C++ification of arena_t-related functions

Categories

(Core :: Memory Allocator, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(22 files)

59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
I started doing this because I thought it would help me for the partition allocation API, but it turns out it doesn't, at least not immediately, without other changes that will have to happen later.

I figured the changes are still useful because they do a lot of changes that align more parts of mozjemalloc to the gecko coding style. But as they might have an impact on how the compiler optimizes things, I won't take chances and will wait for trunk to turn into 58 before actually landing.
Comment on attachment 8909626 [details]
Bug 1401099 - Use Gecko style names for arena_t members.

https://reviewboard.mozilla.org/r/181108/#review186398
Attachment #8909626 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909627 [details]
Bug 1401099 - Move arena_purge to a method of arena_t.

https://reviewboard.mozilla.org/r/181110/#review186400
Attachment #8909627 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909628 [details]
Bug 1401099 - Move hard_purge_arena to a method of arena_t.

https://reviewboard.mozilla.org/r/181112/#review186402
Attachment #8909628 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909629 [details]
Bug 1401099 - Move arena_new to a method of arena_t.

https://reviewboard.mozilla.org/r/181114/#review186404
Attachment #8909629 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909630 [details]
Bug 1401099 - Move arena_chunk_init to a method of arena_t.

https://reviewboard.mozilla.org/r/181116/#review186408

::: memory/build/mozjemalloc.cpp:2658
(Diff revision 1)
>  	if (chunk->ndirty == 0 && old_ndirty > 0)
>  		arena_chunk_tree_dirty_remove(&arena->mChunksDirty, chunk);
>  }
>  
> -static void
> -arena_chunk_init(arena_t *arena, arena_chunk_t *chunk, bool zeroed)
> +void
> +arena_t::ChunkInit(arena_chunk_t* aChunk, bool aZeroed)

This one feels like it would be better as a arena_chunk_t::Init().
Attachment #8909630 - Flags: review?(n.nethercote) → review-
Comment on attachment 8909632 [details]
Bug 1401099 - Move arena_run_alloc to a method of arena_t.

https://reviewboard.mozilla.org/r/181120/#review186420

::: memory/build/mozjemalloc.cpp:3286
(Diff revision 1)
>  	void *ret;
>  
>  	/* Large allocation. */
>  	size = PAGE_CEILING(size);
>  	malloc_spin_lock(&arena->mLock);
> -	ret = (void *)arena_run_alloc(arena, nullptr, size, true, zero);
> +	ret = (void *)arena->RunAlloc(nullptr, size, true, zero);

'(void*)'

::: memory/build/mozjemalloc.cpp:3353
(Diff revision 1)
>  
>  	MOZ_ASSERT((size & pagesize_mask) == 0);
>  	MOZ_ASSERT((alignment & pagesize_mask) == 0);
>  
>  	malloc_spin_lock(&arena->mLock);
> -	ret = (void *)arena_run_alloc(arena, nullptr, alloc_size, true, false);
> +	ret = (void *)arena->RunAlloc(nullptr, alloc_size, true, false);

ditto
Attachment #8909632 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909633 [details]
Bug 1401099 - Move arena_run_dalloc to a method of arena_t.

https://reviewboard.mozilla.org/r/181122/#review186422

::: memory/build/mozjemalloc.cpp:2912
(Diff revision 1)
>  {
> -	arena_chunk_t *chunk;
> +  arena_chunk_t* chunk;
> -        size_t size, run_ind, run_pages;
> +  size_t size, run_ind, run_pages;
>  
> -	chunk = (arena_chunk_t *)CHUNK_ADDR2BASE(run);
> -	run_ind = (size_t)(((uintptr_t)run - (uintptr_t)chunk)
> +  chunk = (arena_chunk_t*)CHUNK_ADDR2BASE(aRun);
> +  run_ind = (size_t)(((uintptr_t)aRun - (uintptr_t)chunk)

You could do `uintptr_t(aRun)`-style casts.
Attachment #8909633 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909634 [details]
Bug 1401099 - Move arena_run_split to a method of arena_t.

https://reviewboard.mozilla.org/r/181124/#review186424

::: memory/build/mozjemalloc.cpp:788
(Diff revision 1)
>  public:
>    arena_run_t* RunAlloc(arena_bin_t* aBin, size_t aSize, bool aLarge, bool aZero);
>  
>    void RunDalloc(arena_run_t* aRun, bool aDirty);
>  
> +  void RunSplit(arena_run_t* aRun, size_t aSize, bool aLarge, bool aZero);

Would SplitRun() be a better name? (Likewise DallocRun()?)
Attachment #8909634 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909635 [details]
Bug 1401099 - Move arena_run_trim_head to a method of arena_t.

https://reviewboard.mozilla.org/r/181126/#review186774

::: memory/build/mozjemalloc.cpp:790
(Diff revision 1)
>  
>    void RunDalloc(arena_run_t* aRun, bool aDirty);
>  
>    void RunSplit(arena_run_t* aRun, size_t aSize, bool aLarge, bool aZero);
>  
> +  void RunTrimHead(arena_chunk_t* aChunk, arena_run_t* aRun, size_t aOldSize, size_t aNewSize);

TrimRunHead?
Attachment #8909635 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #28)
> Comment on attachment 8909632 [details]
> Bug 1401099 - Move arena_run_alloc to a method of arena_t.
> 
> https://reviewboard.mozilla.org/r/181120/#review186420
> 
> ::: memory/build/mozjemalloc.cpp:3286
> (Diff revision 1)
> >  	void *ret;
> >  
> >  	/* Large allocation. */
> >  	size = PAGE_CEILING(size);
> >  	malloc_spin_lock(&arena->mLock);
> > -	ret = (void *)arena_run_alloc(arena, nullptr, size, true, zero);
> > +	ret = (void *)arena->RunAlloc(nullptr, size, true, zero);
> 
> '(void*)'

How about removing those (void*) casts?
(In reply to Nicholas Nethercote [:njn] from comment #30)
> Would SplitRun() be a better name? (Likewise DallocRun()?)

And AllocRun?
> How about removing those (void*) casts?

Even better!
Comment on attachment 8909636 [details]
Bug 1401099 - Move arena_run_trim_tail to a method of arena_t.

https://reviewboard.mozilla.org/r/181128/#review186808

::: memory/build/mozjemalloc.cpp:792
(Diff revision 1)
>  
>    void RunSplit(arena_run_t* aRun, size_t aSize, bool aLarge, bool aZero);
>  
>    void RunTrimHead(arena_chunk_t* aChunk, arena_run_t* aRun, size_t aOldSize, size_t aNewSize);
>  
> +  void RunTrimTail(arena_chunk_t* aChunk, arena_run_t* aRun, size_t aOldSize, size_t aNewSize, bool dirty);

TrimRunTail()?
Attachment #8909636 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909637 [details]
Bug 1401099 - Move arena_bin_malloc_easy to a method of arena_t.

https://reviewboard.mozilla.org/r/181130/#review186820

::: memory/build/mozjemalloc.cpp:3116
(Diff revision 1)
>  	return (run);
>  }
>  
>  /* bin->runcur must have space available before this function is called. */
> -static inline void *
> -arena_bin_malloc_easy(arena_t *arena, arena_bin_t *bin, arena_run_t *run)
> +void*
> +arena_t::BinMallocEasy(arena_bin_t* aBin, arena_run_t* aRun)

MallocBinEasy?
Attachment #8909637 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909637 [details]
Bug 1401099 - Move arena_bin_malloc_easy to a method of arena_t.

https://reviewboard.mozilla.org/r/181130/#review186820

Or maybe MallocEasyBin?
Comment on attachment 8909638 [details]
Bug 1401099 - Move arena_bin_malloc_hard to a method of arena_t.

https://reviewboard.mozilla.org/r/181132/#review186826

::: memory/build/mozjemalloc.cpp:796
(Diff revision 1)
>  
>    void RunTrimTail(arena_chunk_t* aChunk, arena_run_t* aRun, size_t aOldSize, size_t aNewSize, bool dirty);
>  
>    inline void* BinMallocEasy(arena_bin_t* aBin, arena_run_t* aRun);
>  
> +  void* BinMallocHard(arena_bin_t* aBin);

MallocHardBin?
Attachment #8909638 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909639 [details]
Bug 1401099 - Move arena_bin_nonfull_run_get to a method of arena_t.

https://reviewboard.mozilla.org/r/181134/#review186828

::: memory/build/mozjemalloc.cpp:3068
(Diff revision 1)
>  
>    RunDalloc((arena_run_t*)((uintptr_t)aRun + aNewSize), aDirty);
>  }
>  
> -static arena_run_t *
> -arena_bin_nonfull_run_get(arena_t *arena, arena_bin_t *bin)
> +arena_run_t*
> +arena_t::BinNonFullRunGet(arena_bin_t* aBin)

GetNonFullRunBin?
Attachment #8909639 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909640 [details]
Bug 1401099 - Move arena_malloc_small to a method of arena_t.

https://reviewboard.mozilla.org/r/181136/#review186830
Attachment #8909640 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909641 [details]
Bug 1401099 - Move arena_malloc_large to a method of arena_t.

https://reviewboard.mozilla.org/r/181138/#review186832

::: memory/build/mozjemalloc.cpp:3335
(Diff revision 1)
>  	MOZ_ASSERT(QUANTUM_CEILING(size) <= arena_maxclass);
>  
>  	if (size <= bin_maxclass) {
>  		return (arena->MallocSmall(size, zero));
>  	} else
> -		return (arena_malloc_large(arena, size, zero));
> +		return (arena->MallocLarge(size, zero));

Remove parens. (Likewise for the MallocSmall call above, which you could do in the earlier patch.)
Attachment #8909641 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909642 [details]
Bug 1401099 - Move arena_malloc to a method of arena_t.

https://reviewboard.mozilla.org/r/181140/#review186836

::: memory/build/mozjemalloc.cpp:3335
(Diff revision 1)
> +  MOZ_ASSERT(QUANTUM_CEILING(aSize) <= arena_maxclass);
>  
> -	MOZ_ASSERT(arena);
> -	MOZ_DIAGNOSTIC_ASSERT(arena->mMagic == ARENA_MAGIC);
> -	MOZ_ASSERT(size != 0);
> -	MOZ_ASSERT(QUANTUM_CEILING(size) <= arena_maxclass);
> +  if (aSize <= bin_maxclass) {
> +    return MallocSmall(aSize, aZero);
> +  }
> +  return MallocLarge(aSize, aZero);

I'd use ?: here, because the two return expressions have equal significance.
Attachment #8909642 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909643 [details]
Bug 1401099 - Move arena_palloc to a method of arena_t.

https://reviewboard.mozilla.org/r/181142/#review186838
Attachment #8909643 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909644 [details]
Bug 1401099 - Move arena_dalloc_small to a method of arena_t.

https://reviewboard.mozilla.org/r/181144/#review186840
Attachment #8909644 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909645 [details]
Bug 1401099 - Move arena_dalloc_large to a method of arena_t.

https://reviewboard.mozilla.org/r/181146/#review186842
Attachment #8909645 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909646 [details]
Bug 1401099 - Move arena_ralloc_large_shrink to a method of arena_t.

https://reviewboard.mozilla.org/r/181148/#review186844

::: memory/build/mozjemalloc.cpp:814
(Diff revision 1)
>  
>    inline void DallocSmall(arena_chunk_t* aChunk, void* aPtr, arena_chunk_map_t *aMapElm);
>  
>    void DallocLarge(arena_chunk_t* aChunk, void* aPtr);
>  
> +  void RallocLargeShrink(arena_chunk_t* aChunk, void* aPtr, size_t aSize, size_t aOldSize);

RallocShrinkLarge?
Attachment #8909646 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909647 [details]
Bug 1401099 - Move arena_ralloc_large_grow to a method of arena_t.

https://reviewboard.mozilla.org/r/181150/#review186846

::: memory/build/mozjemalloc.cpp:814
(Diff revision 1)
>  
>    void DallocLarge(arena_chunk_t* aChunk, void* aPtr);
>  
>    void RallocLargeShrink(arena_chunk_t* aChunk, void* aPtr, size_t aSize, size_t aOldSize);
>  
> +  bool RallocLargeGrow(arena_chunk_t* aChunk, void* aPtr, size_t aSize, size_t aOldSize);

RallocGrowLarge? Now I'm not sure about that or RallocShrinkLarge... you choose.
Attachment #8909647 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909631 [details]
Bug 1401099 - Move arena_chunk_dealloc to a method of arena_t.

https://reviewboard.mozilla.org/r/181118/#review186852

::: memory/build/mozjemalloc.cpp:779
(Diff revision 1)
>  
>    bool Init();
>  
>    void ChunkInit(arena_chunk_t* aChunk, bool aZeroed);
>  
> +  void ChunkDealloc(arena_chunk_t* aChunk);

DeallocChunk
Attachment #8909631 - Flags: review?(n.nethercote) → review+
Comment on attachment 8909630 [details]
Bug 1401099 - Move arena_chunk_init to a method of arena_t.

https://reviewboard.mozilla.org/r/181116/#review186854

::: memory/build/mozjemalloc.cpp:774
(Diff revision 1)
>     */
>    arena_bin_t mBins[1]; /* Dynamically sized. */
>  
>    bool Init();
>  
> +  void ChunkInit(arena_chunk_t* aChunk, bool aZeroed);

InitChunk
Attachment #8909630 - Flags: review- → review+
(In reply to Nicholas Nethercote [:njn] from comment #39)
> GetNonFullRunBin?

I went with GetNonFullBinRun.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/1252fd56a1c9
Use Gecko style names for arena_t members. r=njn
https://hg.mozilla.org/integration/autoland/rev/cab516c56578
Move arena_purge to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/d683f3be2720
Move hard_purge_arena to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/77d191082add
Move arena_new to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/d5471a774563
Move arena_chunk_init to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/4dc1af92e208
Move arena_chunk_dealloc to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/297976241b7c
Move arena_run_alloc to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/85d693377437
Move arena_run_dalloc to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/164be858d88c
Move arena_run_split to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/b73ccc81201f
Move arena_run_trim_head to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/c6985d5f90ca
Move arena_run_trim_tail to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/44b578a65ada
Move arena_bin_malloc_easy to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/05baed504ac5
Move arena_bin_malloc_hard to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/ded028a53452
Move arena_bin_nonfull_run_get to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/9aac426a0bb6
Move arena_malloc_small to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/5e4cc07e9af3
Move arena_malloc_large to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/1f0f2e2553d1
Move arena_malloc to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/bc5302021292
Move arena_palloc to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/c581cd625a23
Move arena_dalloc_small to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/bb5f7d39f4b1
Move arena_dalloc_large to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/877f1fbd034c
Move arena_ralloc_large_shrink to a method of arena_t. r=njn
https://hg.mozilla.org/integration/autoland/rev/526fae334c8b
Move arena_ralloc_large_grow to a method of arena_t. r=njn
Depends on: 1406732
You need to log in before you can comment on or make changes to this bug.