Skip to content

Conversation

@Theodus
Copy link
Contributor

@Theodus Theodus commented Feb 2, 2019

No description provided.

@msftclas
Copy link

msftclas commented Feb 2, 2019

CLA assistant check
All CLA requirements met.

@Theodus
Copy link
Contributor Author

Theodus commented Feb 2, 2019

Also, quick question: Should calloc ever result in a call to madvise(... MADV_DONTNEED) on the region being allocated? This seems wrong to me.

@davidchisnall
Copy link
Collaborator

I'm not 100% convinced by this change. realloc is often used in a pattern where you allocate something bigger than you'll need, figure out how much you actually need, and then realloc to shrink the allocation. With malloc implementations that subdivide allocations, this lets the allocator reuse the rest of the allocation. It's not clear to me what the correct behaviour should be with snmalloc: we'll waste memory if we don't copy, but we'll also incur extra overhead.

On the calloc / madvise question, it might make sense on Linux. Linux's MADV_DONTNEED is misnamed and actually removes the physical pages and replaces them with CoW mappings of a zero page. Whether this hurts performance more or less than calling memset is not clear. We'll only hit this code path on allocations that are greater than 4KB, so we'll end up blowing away a chunk of cache if we memset. On the other hand, it's a system call that acquires a bunch of VM subsystem locks (probably does some IPIs), then the same thing on page fault a few instructions later when the allocation is touched. We should possibly consider always memseting the first page and only madviseing later ones.

@mjp41
Copy link
Member

mjp41 commented Feb 2, 2019

We could only copy if the size would lead to a different sizeclass?

"Calling realloc on pointer that is not to the start of an allocation");
}
#endif
size_t sz = SNMALLOC_NAME_MANGLE(malloc_usable_size)(ptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call Alloc::alloc_size directly here, rather than the wrapper?

davidchisnall
davidchisnall previously approved these changes Feb 3, 2019
Copy link
Collaborator

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Minor change would be nice, but I'm happy either way.

@davidchisnall davidchisnall merged commit fc24b8b into microsoft:master Feb 3, 2019
@Theodus Theodus deleted the override branch February 3, 2019 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants