Page MenuHomePhabricator

Speadup memory regions handling and cleanup related code
ClosedPublic

Authored by tatyana-krasnukha on Dec 7 2018, 10:50 PM.

Details

Summary
  • replace unnecessary shared pointers with unique pointers
  • reserve space before filling a vector with 'push_back' in a loop to avoid muptiple allocations and memory fragmentation (except Process::GetMemoryRegions, it is not possible to know the size in advance here)
  • override GetMemoryRegions for the ProcessMinidump:

Process::GetMemoryRegions fills the list of regions by calling GetMemoryRegionInfo until it returns an error. But ProcessMinidump implementation GetMemoryRegionInfo parses whole list every time and searches requested range there. Now GetMemoryRegions does this work just once.

  • use move semantic where it is possible (and desirable)
  • fix size_t -> uint32_t truncation
  • add missing constness

I had to change API slightly, but believe these changes doesn't break existing code that uses this API.
Profiling a sipmle program I got nice results - execution of GetMemoryRegions function took just 0.01% of total time against 0.17% for unchanged version.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

labath added a subscriber: labath.Dec 10 2018, 9:00 AM

Woohoo for speedups.

However, I noticed that you are modifying the signatures of SB functions, which I don't believe will be acceptable for lldb.

include/lldb/API/SBMemoryRegionInfo.h
106

Generally, I think it's better to use values here instead of rvalue references, as they make clearer interfaces (a function which receives the a unique_ptr by value has to consume it whether it likes to or not, but in case of rvalue references, the object will survive the function call unless the function explicitly does something to it).

I don't believe this should matter for performance, as that's just copying the pointer value around.

source/Plugins/Process/minidump/MinidumpParser.cpp
404 ↗(On Diff #177326)

Namespaces are not indented in llvm style.

408 ↗(On Diff #177326)

llvm prefers static functions instead of ones in anonymous namespaces

clayborg requested changes to this revision.Dec 10 2018, 9:52 AM

I am going to stop making comments as I have been working on a similar patch that does more than this one. I will post it today and we can decide which approach we want to use.

include/lldb/API/SBMemoryRegionInfoList.h
29

Revert. You can add functions, but never change them. Our API is stable.

31

This you can add, but std::vector implementations are quite efficient and I doubt this is really needed?

33

Revert to maintain API

35

"const" means nothing to our objects as they have shared pointers or unique pointers which never change.

37

ditto, and revert for API

include/lldb/Target/Process.h
2084

This was a a vector of shared pointers, seemingly for no good reason prior to this change, I would change this to be a vector of MemoryRegionInfo instances?

source/API/SBMemoryRegionInfo.cpp
23–24 ↗(On Diff #177326)

Fine to add to API, but really no one outside LLDB will be able to do anything with this function (nor the one on the left) since MemoryRegionInfo is opaque as far as the public clients are concerned. It is there for internal LLDB convenience. So do we really need this? We can add a method that copies from an instance if needed. MemoryRegionInfo structs are very simple to copy. No need for heroics here.

23–27 ↗(On Diff #177326)

Don't remove... API

39–44 ↗(On Diff #177326)

Do this by instance instead of a move assignment operator with a unique ptr?

source/API/SBMemoryRegionInfoList.cpp
93

revert

97–100

Who needs this externally? I would rather not add this to the public API?

98

revert

118

remove const

123

revert

source/API/SBProcess.cpp
1367

Remove. "sb_region_info" already has an instance we can use.

1367–1368

Change Process::GetMemoryRegionInfo() to use an "MemoryRegionInfo &region" instead of shared pointer or unique pointer?

1367–1368

If above changes are made this is not needed.

1394

Just use the std::vector<MemoryRegionInfo> from MemoryRegionInfoListImpl inside the "sb_region_list" already?

1395

Change Process:: GetMemoryRegions() to take a "std::vector<MemoryRegionInfo>&region_list"?

1396

This doesn't need to be done if the above code is changed

1397–1398

This doesn't need to be done if the above code is changed to just pass the vector of regions from "sb_region_list"

source/Plugins/Process/minidump/MinidumpParser.h
91 ↗(On Diff #177326)

switch to all internal GetMemoryRegions calls to just use "std::vector<lldb::MemoryRegionInfo>"?

This revision now requires changes to proceed.Dec 10 2018, 9:52 AM
zturner added inline comments.
include/lldb/API/SBMemoryRegionInfoList.h
31

std::vector is as efficient as it can be without additional hints from the user, but reserve is a very useful optimization in general.

See my patch: https://reviews.llvm.org/D55522

Let me know what you think

clayborg added inline comments.Dec 10 2018, 10:40 AM
include/lldb/API/SBMemoryRegionInfoList.h
31

Agreed but this doesn't need to be in the public API as there is no real need.

Thanks to all for comments!

As I wrote in inline comments, 'non-const' -> 'const' changes don't seem to break anything, I'd like to leave it here. Correct me, if I'm wrong.

The second question is whether it is allowed to change private member functions of SBXxx classes?

include/lldb/API/SBMemoryRegionInfoList.h
31

I'm not aware of the usual count of memory regions in dump files, but when I tested on a file with 140 regions the difference was significant
(if take into account all loops that do push_back without previous reserve).

35

Non-const argument version prevents passing r-value in this function. Another reason is readability.
Is this change braking API too?

include/lldb/Target/Process.h
2084

Hah, I feel stupid now)

source/API/SBMemoryRegionInfo.cpp
23–24 ↗(On Diff #177326)

This is instead of private constructor with a raw pointer

23–27 ↗(On Diff #177326)

Private member functions cannot be changed too?

Replace vector of unique pointers with vector of values, revert some API changes, remove MinidumpParser changes.
I will put overridden GetMemoryRegions in a separate patch.

tatyana-krasnukha marked an inline comment as done.

Addressed more comments

clayborg accepted this revision.Dec 18 2018, 1:21 PM
This revision is now accepted and ready to land.Dec 18 2018, 1:21 PM

Minor change: replace using with typedef since old swig versions don't understand c++11's aliases.

clayborg requested changes to this revision.Dec 19 2018, 9:24 AM

One quick change required to avoid STL in public headers.

include/lldb/lldb-forward.h
16

Remove due to this header being part of the public API.

135

After thinking about this a bit, we shouldn't put any STL stuff in headers that are part of the public API as we never want to expose any STL through our API. Can probably move this into MemoryRegionInfo.h, or just use "std::vector<lldb_private::MemoryRegionInfo>" everywhere as long as it is in lldb_private or the global namespace (we only export "lldb::*").

This revision now requires changes to proceed.Dec 19 2018, 9:24 AM

Rid off of including std::vector at the cost of adding a wrapper class that can be forward declared.
Without this, it is not possible to do https://reviews.llvm.org/D55472#inline-491159

clayborg accepted this revision.Dec 19 2018, 12:24 PM
This revision is now accepted and ready to land.Dec 19 2018, 12:24 PM
This revision was automatically updated to reflect the committed changes.