This is an archive of the discontinued LLVM Phabricator instance.

Increase default memory cache line size for android
ClosedPublic

Authored by labath on Oct 16 2015, 8:22 AM.

Details

Summary

ADB packets have a fixed size of 4k. This means the size of memory reads does not affect speed
too much (as long as it fits in one packet). Therefore, I am increasing the default memory read
size for android to 2k. This value is used only if the user has not modified the default
memory-cache-line-size setting.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 37588.Oct 16 2015, 8:22 AM
labath retitled this revision from to Increase default memory cache line size for android.
labath updated this object.
labath added reviewers: clayborg, tberghammer.
labath added a subscriber: lldb-commits.
clayborg requested changes to this revision.Oct 16 2015, 1:14 PM
clayborg edited edge metadata.

I would rather you be able to ask the current platform for the default memory cache line size and have lldb_private::Process do this automatically on the first memory read if the process property wasn't manually set. So my inline comments will reflect this notion.

include/lldb/Target/Process.h
70–72 ↗(On Diff #37588)

Remove

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
148 ↗(On Diff #37588)

Remove the call to AdjustProcessProperties(...) and let the process call platform_sp->GetDefaultMemoryCacheLineSize() when it needs to. If the only this this PlatformAndroidRemoteGDBServer::DebugProcess() was doing was to allow calling AdjustProcessProperties() then remove this whole function.

159 ↗(On Diff #37588)

Remove the call to AdjustProcessProperties(...) and let the process call platform_sp->GetDefaultMemoryCacheLineSize() when it needs to. If the only this this PlatformAndroidRemoteGDBServer::Attach() was doing was to allow calling AdjustProcessProperties() then remove this whole function.

163–174 ↗(On Diff #37588)

Change this to be:

uint32_t
PlatformAndroidRemoteGDBServer:: GetDefaultMemoryCacheLineSize()
{
    return g_android_default_cache_size;
}

Add the following to Platform.h inside the lldb_private::Platform class definition:

virtual uint32_t GetDefaultMemoryCacheLineSize() { return 0; }
source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h
39–49 ↗(On Diff #37588)

If the only this these fucntions were added for was to allow calling AdjustProcessProperties() then remove these.

55–56 ↗(On Diff #37588)

Change to be:

uint32_t GetDefaultMemoryCacheLineSize() override;
source/Target/Process.cpp
183–189 ↗(On Diff #37588)

Remove this and modify Process to fetch this information on attach if the process option wasn't set. This means you don't need to expose this OptionValueSP and you can do it all within the process by grabbing the platform from the target and then calling platform_sp->GetDefaultMemoryCacheLineSize(). The default platform implementation should return 0 and if zero is returned (meaning no preference), then continue using the current setting that process was using from the setting, else override the current value.

This revision now requires changes to proceed.Oct 16 2015, 1:14 PM
tberghammer edited edge metadata.Oct 19 2015, 5:16 AM

I agree with Greg that the Platform should hand out the value but I would suggest to create a Platform::GetMemoryCacheLineSize() function what will be overwritten in PlatfromAndroid.

ADB packets have a fixed size of 4k.

It is not true. ADB packets have a maximum size of 4k but their actual size is matching with the data transferred (+header). Increasing the packet size won't have measurable effect for the packet transport time because the transport time is bound by the latency because of the lot of round trips and not by the time required to transport the data.

labath updated this revision to Diff 37776.Oct 19 2015, 10:39 AM
labath edited edge metadata.

Adress review comments.

New version of the patch. I agree that it looks better like this.

I have put the setting logic in the process constructor, rather than doing it lazily - if anyone was observing the setting value, it would seem strange that the value of setting changed suddenly after the first memory read.

clayborg accepted this revision.Oct 19 2015, 10:53 AM
clayborg edited edge metadata.

Looks good. The other way to do this would be to add a key/value pair to the qHostInfo like "memory_cache_line_byte_size" and return a value that makes sense for the current Andriod debug target. Why? This change will hard code any android target to use 4K packets. In the future if you have a watch or other Android device that has slow communication, it might be nice to be able to control this by modifying the lldb-server so this can be controlled at a finer level. Just something to think about.

This revision is now accepted and ready to land.Oct 19 2015, 10:53 AM

Looks good with a few minor comments inline

One additional design question:
What is your opinion about specifying the default value for the memory cache line size (the 512 byte) in Platform::GetDefaultMemoryCacheLineSize() instead of in the property definition? I think it makes the interface a bit cleaner and then target.process.memory-cache-line-size can have a default value of 0 (or -1) on LLDB startup and we overwrite it by the value returned by the Platform if it haven't been set to an actual value by the user.

test/android/platform/TestDefaultCacheLineSize.py
15–17 ↗(On Diff #37776)

(nit): You don't need this (called automatically because of the inheritance)

37 ↗(On Diff #37776)

(nit): I think it is a leftover from an other test

This revision was automatically updated to reflect the committed changes.

Good catch. fixed and committed.

I believe adding this to qHostInfo is over-engineering at the moment. But it's a good idea, and we can do it if we have a need for more control of this in the future.