This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Use our own combined allocator
ClosedPublic

Authored by cryptoad on May 9 2017, 9:52 AM.

Details

Summary

The reasoning behind this change is twofold:

  • the current combined allocator (sanitizer_allocator_combined.h) implements features that are not relevant for Scudo, making some code redundant, and some restrictions not pertinent (alignments for example). This forced us to do some weird things between the frontend and our secondary to make things work;
  • we have enough information to be able to know if a chunk will be serviced by the Primary or Secondary, allowing us to avoid extraneous calls to functions such as PointerIsMine or CanAllocate.

As a result, the new scudo-specific combined allocator is very straightforward,
and allows us to remove some now unnecessary code both in the frontend and the
secondary. Unused functions have been left in as unimplemented for now.

It turns out to also be a sizeable performance gain (3% faster in some Android
memory_replay benchmarks, doing some more on other platforms).

Event Timeline

cryptoad created this revision.May 9 2017, 9:52 AM
cryptoad updated this revision to Diff 98532.May 10 2017, 2:22 PM

Small change in the Secondary.

dvyukov edited edge metadata.May 10 2017, 2:30 PM

Looks fine to me. Aleksey, do you want to take a look?

alekseyshl added inline comments.May 10 2017, 2:36 PM
lib/scudo/scudo_allocator.cpp
367

Add space before ":"

lib/scudo/scudo_allocator_combined.h
148

Are those UNIMPLEMENTED going to be implemented at some point?

cryptoad marked an inline comment as done.May 10 2017, 2:42 PM
cryptoad added inline comments.
lib/scudo/scudo_allocator_combined.h
148

To be perfectly honest, I am not sure.
I think this will mostly depend on needs expressed by the users, but there is no specific timeframe.
For example, they might be a need to walk all the allocator chunks for debugging purposes, which would involve implementing a bunch of them both on the Combined and the Secondary.

cryptoad updated this revision to Diff 98535.May 10 2017, 2:43 PM

Addressing Aleksey's comment.

alekseyshl edited edge metadata.May 10 2017, 3:28 PM

I do not like the code duplication and all those UNIMPLEMENTED in ScudoCombinedAllocator and around, it's hard to reason why there's so much unused API (why is it declared then?) Also I expected to see some simplification in CombinedAllocator<>, none was necessary, maybe it means that CombinedAllocator has some potential for improvement, to be more flexible to address Scudo needs?

All that said, 3% performance gain is too big to pass on, if CombinedAllocator is not possible or too complicated to adjust for Scudo, I'm ok with this change.

I do not like the code duplication and all those UNIMPLEMENTED in ScudoCombinedAllocator and around, it's hard to reason why there's so much unused API (why is it declared then?) Also I expected to see some simplification in CombinedAllocator<>, none was necessary, maybe it means that CombinedAllocator has some potential for improvement, to be more flexible to address Scudo needs?

It's fair. I'll send out a revision removing all the unneeded code from the combined and the secondary.
As for modifying the original combined, I do not think it's doable without a significant increase in complexity and branches, which is what I am trying to get rid of.
A lot of the difference comes from the fact that the frontend of the other sanitizers defer a lot to the original combined (alignment, realloc, memset, etc), while our frontend does all that work.

cryptoad updated this revision to Diff 98646.May 11 2017, 9:35 AM

Removing all the unused code from the Secondary and the Combined allocator.
The resulting Combined is tiny.
I also rearranged some code to make it clearer (at least to me).

alekseyshl accepted this revision.May 11 2017, 2:07 PM
This revision is now accepted and ready to land.May 11 2017, 2:07 PM
cryptoad closed this revision.May 11 2017, 2:53 PM