This is an archive of the discontinued LLVM Phabricator instance.

[clang][extract-api] Stop allocating APIRecords via BumpPtrAllocator
ClosedPublic

Authored by dang on Mar 23 2022, 10:20 AM.

Details

Summary

Using a BumpPtrAllocator introduced memory leaks for APIRecords as they
contain a std::vector. This meant that we needed to always keep a
reference to the records in APISet and arrange for their destructor to
get called appropriately. This was further complicated by the need for
records to own sub-records as these subrecords would still need to be
allocated via the BumpPtrAllocator and the owning record would now need
to arrange for the destructor of its subrecords to be called
appropriately.

Since APIRecords contain a std::vector so whenever elements get added to
that there is an associated heap allocation regardless. Since
performance isn't currently our main priority it makes sense to use
regular unique_ptr to keep track of APIRecords, this way we don't need
to arrange for destructors to get called.

The BumpPtrAllocator is still used for strings such as USRs so that we
can easily de-duplicate them as necessary.

Diff Detail

Event Timeline

dang created this revision.Mar 23 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2022, 10:20 AM
dang requested review of this revision.Mar 23 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2022, 10:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
zixuw added a comment.Mar 23 2022, 1:38 PM

This is much cleaner now. Thanks Daniel!

clang/include/clang/ExtractAPI/API.h
326

Should we just rename the allocator along the lines of StringAllocator or StringPool so that the code is self-explanatory?

326

Also now that the allocator is used exclusively for strings, should we use SpecificBumpPtrAllocator?

dang added a comment.Mar 24 2022, 3:55 AM

I have checked locally against San to make sure this doesn't reintroduce the memory leak issues we were seeing initially that lead to the addition of APIRecordUniquePtr

dang updated this revision to Diff 417885.Mar 24 2022, 5:03 AM
dang marked 2 inline comments as done.

Address review feedback: Rename APISet::Allocator to APISet::StringAllocator

clang/include/clang/ExtractAPI/API.h
326

After thinking about it, we shouldn't because we are storing strings that shouldn't be modified as const char * so we shouldn't need to call destructors.

zixuw accepted this revision.Mar 24 2022, 10:27 AM
This revision is now accepted and ready to land.Mar 24 2022, 10:27 AM