[Lldb-commits] [lldb] 14f4430 - [nfc] [lldb] Prevent needless copies of DataExtractor
Jan Kratochvil via lldb-commits
lldb-commits at lists.llvm.org
Wed Aug 4 11:35:58 PDT 2021
Author: Jan Kratochvil
Date: 2021-08-04T20:35:53+02:00
New Revision: 14f443030c1acc4346589aee3c1c532dc00eae3a
URL: https://github.com/llvm/llvm-project/commit/14f443030c1acc4346589aee3c1c532dc00eae3a
DIFF: https://github.com/llvm/llvm-project/commit/14f443030c1acc4346589aee3c1c532dc00eae3a.diff
LOG: [nfc] [lldb] Prevent needless copies of DataExtractor
lldb_private::DataExtractor contains DataBufferSP m_data_sp which is
relatively expensive to copy (due to multi-threading locking).
llvm::DataExtractor does not have this problem as it uses StringRef
instead.
The copy constructor is explicit as otherwise it is easy to make
unintended modification of a local copy instead of a caller's instance
(D107470 but that is llvm::DataExtractor).
Reviewed By: clayborg
Differential Revision: https://reviews.llvm.org/D107485
Added:
Modified:
lldb/include/lldb/DataFormatters/StringPrinter.h
lldb/include/lldb/Utility/DataExtractor.h
lldb/source/DataFormatters/StringPrinter.cpp
lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
lldb/unittests/DataFormatter/StringPrinterTests.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/DataFormatters/StringPrinter.h b/lldb/include/lldb/DataFormatters/StringPrinter.h
index 4a6e2e9051bff..cb6cba1e254f2 100644
--- a/lldb/include/lldb/DataFormatters/StringPrinter.h
+++ b/lldb/include/lldb/DataFormatters/StringPrinter.h
@@ -133,9 +133,9 @@ class StringPrinter {
ReadBufferAndDumpToStreamOptions(
const ReadStringAndDumpToStreamOptions &options);
- void SetData(DataExtractor d) { m_data = d; }
+ void SetData(DataExtractor &&d) { m_data = std::move(d); }
- lldb_private::DataExtractor GetData() const { return m_data; }
+ const lldb_private::DataExtractor &GetData() const { return m_data; }
void SetIsTruncated(bool t) { m_is_truncated = t; }
diff --git a/lldb/include/lldb/Utility/DataExtractor.h b/lldb/include/lldb/Utility/DataExtractor.h
index 0923e5280cba9..dbf0bce8c8d0d 100644
--- a/lldb/include/lldb/Utility/DataExtractor.h
+++ b/lldb/include/lldb/Utility/DataExtractor.h
@@ -134,7 +134,12 @@ class DataExtractor {
DataExtractor(const DataExtractor &data, lldb::offset_t offset,
lldb::offset_t length, uint32_t target_byte_size = 1);
- DataExtractor(const DataExtractor &rhs);
+ /// Copy constructor.
+ ///
+ /// The copy constructor is explicit as otherwise it is easy to make
+ /// unintended modification of a local copy instead of a caller's instance.
+ /// Also a needless copy of the \a m_data_sp shared pointer is/ expensive.
+ explicit DataExtractor(const DataExtractor &rhs);
/// Assignment operator.
///
@@ -149,6 +154,12 @@ class DataExtractor {
/// A const reference to this object.
const DataExtractor &operator=(const DataExtractor &rhs);
+ /// Move constructor and move assignment operators to complete the rule of 5.
+ ///
+ /// They would get deleted as we already defined those of rule of 3.
+ DataExtractor(DataExtractor &&rhs) = default;
+ DataExtractor &operator=(DataExtractor &&rhs) = default;
+
/// Destructor
///
/// If this object contains a valid shared data reference, the reference
@@ -1005,7 +1016,8 @@ class DataExtractor {
uint32_t m_addr_size; ///< The address size to use when extracting addresses.
/// The shared pointer to data that can be shared among multiple instances
lldb::DataBufferSP m_data_sp;
- const uint32_t m_target_byte_size = 1;
+ /// Making it const would require implementation of move assignment operator.
+ uint32_t m_target_byte_size = 1;
};
} // namespace lldb_private
diff --git a/lldb/source/DataFormatters/StringPrinter.cpp b/lldb/source/DataFormatters/StringPrinter.cpp
index 0c6438f7dd868..6def893ae5ca0 100644
--- a/lldb/source/DataFormatters/StringPrinter.cpp
+++ b/lldb/source/DataFormatters/StringPrinter.cpp
@@ -475,11 +475,9 @@ static bool ReadEncodedBufferAndDumpToStream(
return true;
}
- DataExtractor data(buffer_sp, process_sp->GetByteOrder(),
- process_sp->GetAddressByteSize());
-
StringPrinter::ReadBufferAndDumpToStreamOptions dump_options(options);
- dump_options.SetData(data);
+ dump_options.SetData(DataExtractor(buffer_sp, process_sp->GetByteOrder(),
+ process_sp->GetAddressByteSize()));
dump_options.SetSourceSize(sourceSize);
dump_options.SetIsTruncated(is_truncated);
dump_options.SetNeedsZeroTermination(needs_zero_terminator);
diff --git a/lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp b/lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
index 41bbd2b01a1e7..45c0cd213e0e6 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp
@@ -168,7 +168,7 @@ bool lldb_private::formatters::Char8SummaryProvider(
stream.Printf("%s ", value.c_str());
StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
- options.SetData(data);
+ options.SetData(std::move(data));
options.SetStream(&stream);
options.SetPrefixToken("u8");
options.SetQuote('\'');
@@ -194,7 +194,7 @@ bool lldb_private::formatters::Char16SummaryProvider(
stream.Printf("%s ", value.c_str());
StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
- options.SetData(data);
+ options.SetData(std::move(data));
options.SetStream(&stream);
options.SetPrefixToken("u");
options.SetQuote('\'');
@@ -220,7 +220,7 @@ bool lldb_private::formatters::Char32SummaryProvider(
stream.Printf("%s ", value.c_str());
StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
- options.SetData(data);
+ options.SetData(std::move(data));
options.SetStream(&stream);
options.SetPrefixToken("U");
options.SetQuote('\'');
@@ -254,7 +254,7 @@ bool lldb_private::formatters::WCharSummaryProvider(
const uint32_t wchar_size = *size;
StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
- options.SetData(data);
+ options.SetData(std::move(data));
options.SetStream(&stream);
options.SetPrefixToken("L");
options.SetQuote('\'');
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
index 8eda422f31454..b9aef0ae7d9eb 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -686,7 +686,7 @@ bool lldb_private::formatters::LibcxxWStringSummaryProvider(
if (!wchar_t_size)
return false;
- options.SetData(extractor);
+ options.SetData(std::move(extractor));
options.SetStream(&stream);
options.SetPrefixToken("L");
options.SetQuote('"');
@@ -743,12 +743,14 @@ bool LibcxxStringSummaryProvider(ValueObject &valobj, Stream &stream,
}
}
- DataExtractor extractor;
- const size_t bytes_read = location_sp->GetPointeeData(extractor, 0, size);
- if (bytes_read < size)
- return false;
+ {
+ DataExtractor extractor;
+ const size_t bytes_read = location_sp->GetPointeeData(extractor, 0, size);
+ if (bytes_read < size)
+ return false;
- options.SetData(extractor);
+ options.SetData(std::move(extractor));
+ }
options.SetStream(&stream);
if (prefix_token.empty())
options.SetPrefixToken(nullptr);
diff --git a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
index 12bc7390c7294..79da46d41c4fd 100644
--- a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -519,9 +519,8 @@ ProcessElfCore::parseSegment(const DataExtractor &segment) {
size_t note_start = offset;
size_t note_size = llvm::alignTo(note.n_descsz, 4);
- DataExtractor note_data(segment, note_start, note_size);
- result.push_back({note, note_data});
+ result.push_back({note, DataExtractor(segment, note_start, note_size)});
offset += note_size;
}
@@ -897,7 +896,8 @@ llvm::Error ProcessElfCore::parseLinuxNotes(llvm::ArrayRef<CoreNote> notes) {
/// A note segment consists of one or more NOTE entries, but their types and
/// meaning
diff er depending on the OS.
llvm::Error ProcessElfCore::ParseThreadContextsFromNoteSegment(
- const elf::ELFProgramHeader &segment_header, DataExtractor segment_data) {
+ const elf::ELFProgramHeader &segment_header,
+ const DataExtractor &segment_data) {
assert(segment_header.p_type == llvm::ELF::PT_NOTE);
auto notes_or_error = parseSegment(segment_data);
diff --git a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.h b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
index d8e3cc9ae3e12..39a7394221674 100644
--- a/lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
+++ b/lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
@@ -148,7 +148,7 @@ class ProcessElfCore : public lldb_private::PostMortemProcess {
// Parse thread(s) data structures(prstatus, prpsinfo) from given NOTE segment
llvm::Error ParseThreadContextsFromNoteSegment(
const elf::ELFProgramHeader &segment_header,
- lldb_private::DataExtractor segment_data);
+ const lldb_private::DataExtractor &segment_data);
// Returns number of thread contexts stored in the core file
uint32_t GetNumThreadContexts();
diff --git a/lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp b/lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
index 0c21c0f50abbf..6f3bf02cd303b 100644
--- a/lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
+++ b/lldb/source/Plugins/Process/elf-core/RegisterUtilities.cpp
@@ -34,5 +34,5 @@ DataExtractor lldb_private::getRegset(llvm::ArrayRef<CoreNote> Notes,
uint32_t Type = *TypeOr;
auto Iter = llvm::find_if(
Notes, [Type](const CoreNote &Note) { return Note.info.n_type == Type; });
- return Iter == Notes.end() ? DataExtractor() : Iter->data;
+ return Iter == Notes.end() ? DataExtractor() : DataExtractor(Iter->data);
}
diff --git a/lldb/unittests/DataFormatter/StringPrinterTests.cpp b/lldb/unittests/DataFormatter/StringPrinterTests.cpp
index 84a9372408bbd..a7fa6e8a13190 100644
--- a/lldb/unittests/DataFormatter/StringPrinterTests.cpp
+++ b/lldb/unittests/DataFormatter/StringPrinterTests.cpp
@@ -37,9 +37,8 @@ static Optional<std::string> format(StringRef input,
opts.SetEscapeNonPrintables(true);
opts.SetIgnoreMaxLength(false);
opts.SetEscapeStyle(escape_style);
- DataExtractor extractor(input.data(), input.size(),
- endian::InlHostByteOrder(), sizeof(void *));
- opts.SetData(extractor);
+ opts.SetData(DataExtractor(input.data(), input.size(),
+ endian::InlHostByteOrder(), sizeof(void *)));
const bool success = StringPrinter::ReadBufferAndDumpToStream<elem_ty>(opts);
if (!success)
return llvm::None;
More information about the lldb-commits
mailing list