This is an archive of the discontinued LLVM Phabricator instance.

Use a utility function to speed up "process load image" for POSIX Platforms
ClosedPublic

Authored by jingham on Apr 16 2018, 2:52 PM.

Details

Summary

The original implementation of PlatformPosix::DoLoadImage compiled the expression to load the image & check the error a fresh every time. That was causing performance problems in some cases where lldb had to load a bunch of images into the process.

This patch replaces that mechanism with an lldb UtilityFunction, so we can JIT the required code once per process and then reuse it on subsequent calls. The work to set up a UtilityFunction is simpler than to compile an expression, so even in the case where you only do one load this will be faster. But then if you are doing many it will be much faster.

The only tricky bit in this is that the Platform holds the DoLoadImage code, but the UtilityFunction has to be per process. Rather than trying to get the Platform to track the Targets & Processes that use it (which it does not do at present) I put the Utility function in the Process, and hand it out from there.

I think I got the android dlopen remapping header in there correctly, but I don't have a way to test that, so if somebody on the Android side could check that out, that would be helpful.

Diff Detail

Repository
rL LLVM

Event Timeline

jingham created this revision.Apr 16 2018, 2:52 PM

First round of comments. Thanks for this!

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
1108

why are we ignoring breakpoints?

1108–1111

Where the 2*addr_size is coming from? Probably we should add a comment.

1112

Shouldd we try setting up a slightly more generous timeout?

Commented inline.

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
1108

This wasn't really a breakpoint that was hit by code initiated by the app you are debugging, so it makes more sense to treat it as an internal matter than to report breakpoint hits to the user.

Suppose the user had put a breakpoint on strcmp. Then they did some operation that turned on an instrumentation function, which in turn required the instrumentation code to load a library. You wouldn't want that operation to be interrupted mid-course, and return to the user with a "Stopped at breakpoint 1.1" UI. That would be disconcerting - especially since from the user's perspective the target isn't even running - and the user would have to figure out what she should do to fix the situation.

There really isn' t much benefit to stopping in whatever breakpoints dlopen hit while doing this load operation.

1108–1111

I'll add a comment. I describe the return structure a bit above where I fill in the argument vector - it's two pointers. But I'll make it clear that's what this is.

1112

I didn't change the timeout: 2 seconds is what most of the utility functions use. We haven't had any reports of this being too short.

jasonmolenda edited subscribers, added: lldb-commits; removed: llvm-commits.Apr 16 2018, 8:08 PM
jasonmolenda added a subscriber: jasonmolenda.
jasonmolenda removed a subscriber: jasonmolenda.
jasonmolenda added a subscriber: jasonmolenda.

Thanks for the heads up. I can test the android part on Wednesday, but right now I don't see a reason why that shouldn't work there.

Overall, I like the idea of using the process class for caching some platform resources. If we end up needing to do that more often, we can think of making using some more generic container for that instead of making a pair of getters/setters for each value.

Apart from the low-level inline comments, the one high-level comment I have is that the DoLoadImage function is getting a bit big. I think it would help readability to split it up a bit. At least the get-cached-function-or-build-one-if-it-is-not-present part seems like it could be easily separated out into a helper function.

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
1069–1070

If SetLoadImageUtilityFunction takes a unique_ptr then you can do a SetLoadImageUtilityFunction(std::move(dlopen_utility_func_up)) here.

source/Target/Process.cpp
6251

Will this ever be true? I would not expect one to be able to change the platform of a process mid-session.

6256

I think the argument of this function should be a unique_ptr as well.

clayborg added inline comments.
include/lldb/Target/Process.h
811

Fix comment to say "Set" instead of "Get"

837

I would think that saving utility functions can be used by other plug-ins. It might be nice to change these two functions to take a name + utility function for the setter. So these functions would look like:

void SetCachedUtilityFunction(ConstString name, UtilityFunction *utility_func);
UtilityFunction *GetCachedUtilityFunction(ConstString name);

Then other plug-ins could cache utility function. I would rather not have each new plug-in that needs this to have to add specific accessor functions.

3184

Make this a map of ConstString -> std::unique_ptr<UtilityFunction> if we end up using SetCachedUtilityFunction as described above

I don't think doing this is necessary when we have only one customer, but if we are going to be designing an general purpose storage facility then I don't think we should be using strings (and particularly not ConstStrings) as the lookup keys.

I would propose going for the the token based approach where each customer has a "token" (it can be just a char) and then you use the address of that token as the key.

Besides being faster, it is more auditable as only the thing that has access to the token can manipulate the data. Also the system could be extended in the future to store arbitrary objects in a type-safe way by making a Token<T> template.

I don't think doing this is necessary when we have only one customer, but if we are going to be designing an general purpose storage facility then I don't think we should be using strings (and particularly not ConstStrings) as the lookup keys.

I would propose going for the the token based approach where each customer has a "token" (it can be just a char) and then you use the address of that token as the key.

a "const void *" is fine as the key. In this case it could be the "Platform *" or the pointer to the ConstString name of the platform plug-in.

Besides being faster, it is more auditable as only the thing that has access to the token can manipulate the data. Also the system could be extended in the future to store arbitrary objects in a type-safe way by making a Token<T> template.

That is fine. For the helper functions though, I would rather us implement this now even if we have only one customer as the next person will see the pattern and say "I need to add my own custom functions to add accessors for my utility function", so I would rather do this right in this patch if possible.

I'm not sure what facility we are designing here? If it is only utility functions, I think that is a very narrow use case, and I'm not sure it needs a special purpose facility. I would prefer to wait till I see a second instance for that. After all, this is not SB API, we can change it as we go along. I would prefer to just leave a comment "if you do this again make it more general" to avoid copying the pattern if somebody's ever going to do that.

If it is more general "I'm going to stash something in the process" then we need to know who owns the thing? Will it be enough to just have the process destroy it when it goes away? Do we want to notify the registree? For this particular use case, I know the answer, but this is specifically a cache, which can always be rebuilt if we get nothing from the cache. Is that what we're modeling in general...

I feel like it is premature to design this store till we have some more prospective uses.

source/Target/Process.cpp
6251

Reading through the code I could not convince myself that it could NOT happen. Target::SetPlatform is a function anybody can call at any time. I agree that it would be odd to do so, and we might think about prohibiting it (Target::SetPlatform could return an error, and forbid changing the Platform, if the Target has a live process.)

If everybody agrees that that is a good idea, I can do that and remove this check.

6256

Okay, I'll try that out.

zturner added inline comments.
source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
1023–1024

Can we use the version that returns a std::string? I consider this version unsafe as it might not always return the full path

jingham updated this revision to Diff 142800.Apr 17 2018, 10:55 AM

Address review comments.

I changed over to passing a up to get stored in the process.
I separated the maker code into a separate function.
I changed GetPath versions (the buffer one was in the original, but I agree the std::string one is better.)
I didn't address the "changing platforms mid-stream" comment. If we want to make that a policy, I think it's better to do that separately.
I didn't add a general purpose facility to cache in the Process, since I don't have enough examples to know what to do there.

jingham marked 5 inline comments as done.Apr 17 2018, 10:56 AM

I am fine if we don't want to do the general solution now. LGTM if all is well in the test suite.

I like the idea of leaving some comment to have a record that we discussed making a more generic storage facility, but I don't think we need to do anything else right now. I doubt we will see many new clients for this very soon.

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
1166

Use LLDB_INVALID_IMAGE_TOKEN here?

source/Target/Process.cpp
6251

I see three call in the codebase to SetPlatform. All of them seem to be in the initialization code, though some of them seem to happen after the process is launched (DynamicLoaderDarwinKernel constructor).

So it may not be possible to completely forbid setting the platform this way, but I think we can at least ban changing the platform once it has already been set in a pretty hard way (lldb_assert or even assert). I think a lot of things would currently break if someone started changing the platforms of a running process all of a sudden.

I added a comment, I'll upload the diff with that in a sec.

source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
1166

That wouldn't be right. LLDB_INVALID_IMAGE_TOKEN is the invalid token in lldb's namespace for image loading tokens, which has no relationship to any given platform's invalid token specifier. In fact, LLDB_INVALID_IMAGE_TOKEN is UINT32_MAX, so it is actually different from the POSIX invalid image token.

source/Target/Process.cpp
6251

Not sure. It seems reasonable to make a target, run it against one platform, then switch the platform and run it again against the new platform. I'm not sure I'm comfortable saying that a target can only ever use one platform.

jingham updated this revision to Diff 142814.Apr 17 2018, 12:10 PM

Added a comment describing our discussions for a general "stash this in the process" facility.

The only outstanding question is whether we should enforce "Targets can't change their platforms" in which case we could remove the check in GetLoadImageUtilityFunction. The check does no harm either way, and this doesn't seem the right forum to decide the larger policy question. The testsuite is clean, so I'm going to check this in.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 17 2018, 1:50 PM
This revision was automatically updated to reflect the committed changes.
labath added inline comments.Apr 18 2018, 1:53 AM
source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
1166

I see. Thanks for the explanation.

source/Target/Process.cpp
6251

Yeah, you're right. I suppose that makes sense. In my mind a target was associated with a given platform from the moment it got created (that is how I always do things: platform select, platform connect, and only then target create). I never tried what would happen if I reversed that. So I guess the platform can change during the lifetime of a Target, but I hope that is not the case for the lifetime of a Process.