Increase default memory cache line size for android

[labath]

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.

[clayborg]

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.

[tberghammer]

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]

Adress review comments.

[labath]

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]

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.

[tberghammer]

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

[labath]

Good catch. fixed and committed.

[labath]

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.

lldb/trunk/include/lldb/Target/Platform.h

//                Error &error) = 0;
         /// signal handler routines as needed.  For most Unix platforms, add _sigtramp.
         ///
         /// @return
         ///     A list of symbol names.  The list may be empty.
         //------------------------------------------------------------------
         virtual const std::vector<ConstString> &
         GetTrapHandlerSymbolNames ();
 
-
         //------------------------------------------------------------------
         /// Find a support executable that may not live within in the
         /// standard locations related to LLDB.
         ///
         /// Executable might exist within the Platform SDK directories, or
         /// in standard tool directories within the current IDE that is
         /// running LLDB.
         ///
         /// @param[in] basename
         ///     The basename of the executable to locate in the current
         ///     platform.
         ///
         /// @return
         ///     A FileSpec pointing to the executable on disk, or an invalid
         ///     FileSpec if the executable cannot be found.
         //------------------------------------------------------------------
         virtual FileSpec
         LocateExecutable (const char *basename)
         {
             return FileSpec();
         }
 
+        //------------------------------------------------------------------
+        /// Allow the platform to set preferred memory cache line size. If non-zero (and the user
+        /// has not set cache line size explicitly), this value will be used as the cache line
+        /// size for memory reads.
+        //------------------------------------------------------------------
+        virtual uint32_t
+        GetDefaultMemoryCacheLineSize() { return 0; }
+
     protected:
         bool m_is_host;
         // Set to true when we are able to actually set the OS version while 
         // being connected. For remote platforms, we might set the version ahead
         // of time before we actually connect and this version might change when
         // we actually connect to a remote platform. For the host platform this
         // will be set to the once we call HostInfo::GetOSVersion().
         bool m_os_version_set_while_connected;

lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.h

public:
         GetSdkVersion();
 
         bool
         GetRemoteOSVersion() override;
 
         Error
         DisconnectRemote () override;
 
+        uint32_t
+        GetDefaultMemoryCacheLineSize() override;
+
      protected:
         const char *
         GetCacheHostname () override;
 
         Error
         DownloadModuleSlice (const FileSpec &src_file_spec,
                              const uint64_t src_offset,
                              const uint64_t src_size,

lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp

 #include "PlatformAndroid.h"
 #include "PlatformAndroidRemoteGDBServer.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::platform_android;
 
 static uint32_t g_initialize_count = 0;
+static const unsigned int g_android_default_cache_size = 2048; // Fits inside 4k adb packet.
 
 void
 PlatformAndroid::Initialize ()
 {
     PlatformLinux::Initialize ();
 
     if (g_initialize_count++ == 0)
     {
PlatformAndroid::DisconnectRemote()
     {
         m_device_id.clear();
         m_sdk_version = 0;
     }
     return error;
 }
 
 uint32_t
+PlatformAndroid::GetDefaultMemoryCacheLineSize()
+{
+    return g_android_default_cache_size;
+}
+
+uint32_t
 PlatformAndroid::GetSdkVersion()
 {
     if (!IsConnected())
         return 0;
 
     if (m_sdk_version != 0)
         return m_sdk_version;
 

lldb/trunk/source/Target/Process.cpp

m_private_state_listener.StartListeningForEvents(&m_private_state_broadcaster,
                                                      eBroadcastBitInterrupt);
 
     m_private_state_listener.StartListeningForEvents(&m_private_state_control_broadcaster,
                                                      eBroadcastInternalStateControlStop |
                                                      eBroadcastInternalStateControlPause |
                                                      eBroadcastInternalStateControlResume);
     // We need something valid here, even if just the default UnixSignalsSP.
     assert (m_unix_signals_sp && "null m_unix_signals_sp after initialization");
+
+    // Allow the platform to override the default cache line size
+    OptionValueSP value_sp =
+        m_collection_sp->GetPropertyAtIndex(nullptr, true, ePropertyMemCacheLineSize)->GetValue();
+    uint32_t platform_cache_line_size = target_sp->GetPlatform()->GetDefaultMemoryCacheLineSize();
+    if (! value_sp->OptionWasSet() && platform_cache_line_size != 0)
+        value_sp->SetUInt64Value(platform_cache_line_size);
 }
 
 //----------------------------------------------------------------------
 // Destructor
 //----------------------------------------------------------------------
 Process::~Process()
 {
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_OBJECT));

lldb/trunk/test/android/platform/Makefile

+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+include $(LEVEL)/Makefile.rules

lldb/trunk/test/android/platform/TestDefaultCacheLineSize.py

+"""
+Verify the default cache line size for android targets
+"""
+
+import os
+import unittest2
+import lldb
+from lldbtest import *
+import lldbutil
+
+class DefaultCacheLineSizeTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipUnlessPlatform(['android'])
+    def test_cache_line_size(self):
+        self.build(dictionary=self.getBuildFlags())
+        exe = os.path.join(os.getcwd(), "a.out")
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target and target.IsValid(), "Target is valid")
+
+        breakpoint = target.BreakpointCreateByName("main")
+        self.assertTrue(breakpoint and breakpoint.IsValid(), "Breakpoint is valid")
+
+        # Run the program.
+        process = target.LaunchSimple(None, None, self.get_process_working_directory())
+        self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
+        self.assertEqual(process.GetState(), lldb.eStateStopped, PROCESS_STOPPED)
+
+        # check the setting value
+        self.expect("settings show target.process.memory-cache-line-size", patterns=[" = 2048"])
+
+        # Run to completion.
+        process.Continue()
+        self.assertEqual(process.GetState(), lldb.eStateExited, PROCESS_EXITED)
+
+if __name__ == '__main__':
+    import atexit
+    lldb.SBDebugger.Initialize()
+    atexit.register(lambda: lldb.SBDebugger.Terminate())
+    unittest2.main()

lldb/trunk/test/android/platform/main.cpp

+//===-- main.cpp ------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+int main ()
+{
+    return 0;
+}