diff --git base/values.cc base/values.cc index 6f3a9e2cd8a2..d35972814034 100644 --- base/values.cc +++ base/values.cc @@ -22,20 +22,6 @@ namespace base { -// base::Value must be standard layout to guarantee that writing to -// |bool_type_| then reading |type_| is defined behaviour. See: -// -// [class.union]: -// If a standard-layout union contains several standard-layout structs that -// share a common initial sequence (9.2), and if an object of this -// standard-layout union type contains one of the standard-layout structs, -// it is permitted to inspect the common initial sequence of any of -// standard-layout struct members; -// -static_assert(std::is_standard_layout::value, - "base::Value should be a standard-layout C++ class in order " - "to avoid undefined behaviour in its implementation!"); - namespace { const char* const kTypeNames[] = {"null", "boolean", "integer", "double", @@ -150,15 +136,15 @@ Value::Value(Type type) : type_(type) { } Value::Value(bool in_bool) - : bool_type_(Type::BOOLEAN), + : type_(Type::BOOLEAN), bool_value_(in_bool) {} Value::Value(int in_int) - : int_type_(Type::INTEGER), + : type_(Type::INTEGER), int_value_(in_int) {} Value::Value(double in_double) - : double_type_(Type::DOUBLE), + : type_(Type::DOUBLE), double_value_(in_double) { if (!std::isfinite(double_value_)) { NOTREACHED() << "Non-finite (i.e. NaN or positive/negative infinity) " @@ -172,7 +158,7 @@ Value::Value(const char* in_string) : Value(std::string(in_string)) {} Value::Value(StringPiece in_string) : Value(std::string(in_string)) {} Value::Value(std::string&& in_string) noexcept - : string_type_(Type::STRING), + : type_(Type::STRING), string_value_(std::move(in_string)) { DCHECK(IsStringUTF8(string_value_)); } @@ -182,19 +168,19 @@ Value::Value(const char16* in_string16) : Value(StringPiece16(in_string16)) {} Value::Value(StringPiece16 in_string16) : Value(UTF16ToUTF8(in_string16)) {} Value::Value(const std::vector& in_blob) - : binary_type_(Type::BINARY), + : type_(Type::BINARY), binary_value_(in_blob.begin(), in_blob.end()) {} Value::Value(base::span in_blob) - : binary_type_(Type::BINARY), + : type_(Type::BINARY), binary_value_(in_blob.begin(), in_blob.end()) {} Value::Value(BlobStorage&& in_blob) noexcept - : binary_type_(Type::BINARY), + : type_(Type::BINARY), binary_value_(std::move(in_blob)) {} Value::Value(const DictStorage& in_dict) - : dict_type_(Type::DICTIONARY), dict_() { + : type_(Type::DICTIONARY), dict_() { dict_.reserve(in_dict.size()); for (const auto& it : in_dict) { dict_.try_emplace(dict_.end(), it.first, @@ -203,17 +189,17 @@ Value::Value(const DictStorage& in_dict) } Value::Value(DictStorage&& in_dict) noexcept - : dict_type_(Type::DICTIONARY), + : type_(Type::DICTIONARY), dict_(std::move(in_dict)) {} -Value::Value(const ListStorage& in_list) : list_type_(Type::LIST), list_() { +Value::Value(const ListStorage& in_list) : type_(Type::LIST), list_() { list_.reserve(in_list.size()); for (const auto& val : in_list) list_.emplace_back(val.Clone()); } Value::Value(ListStorage&& in_list) noexcept - : list_type_(Type::LIST), + : type_(Type::LIST), list_(std::move(in_list)) {} Value& Value::operator=(Value&& that) noexcept { diff --git base/values.h base/values.h index 7bc355ee586d..441967681a4b 100644 --- base/values.h +++ base/values.h @@ -390,78 +390,18 @@ class BASE_EXPORT Value { size_t EstimateMemoryUsage() const; protected: - // Technical note: - // The naive way to implement a tagged union leads to wasted bytes - // in the object on CPUs like ARM ones, which impose an 8-byte alignment - // for double values. I.e. if one does something like: - // - // struct TaggedValue { - // int type_; // size = 1, align = 4 - // union { - // bool bool_value_; // size = 1, align = 1 - // int int_value_; // size = 4, align = 4 - // double double_value_; // size = 8, align = 8 - // std::string string_value_; // size = 12, align = 4 (32-bit) - // }; - // }; - // - // The end result is that the union will have an alignment of 8, and a size - // of 16, due to 4 extra padding bytes following |string_value_| to respect - // the alignment requirement. - // - // As a consequence, the struct TaggedValue will have a size of 24 bytes, - // due to the size of the union (16), the size of |type_| (4) and 4 bytes - // of padding between |type_| and the union to respect its alignment. - // - // This means 8 bytes of unused memory per instance on 32-bit ARM! - // - // To reclaim these, a union of structs is used instead, in order to ensure - // that |double_value_| below is always located at an offset that is a - // multiple of 8, relative to the start of the overall data structure. - // - // Each struct must declare its own |type_| field, which must have a different - // name, to appease the C++ compiler. - // - // Using this technique sizeof(base::Value) == 16 on 32-bit ARM instead - // of 24, without losing any information. Results are unchanged for x86, - // x86_64 and arm64 (16, 32 and 32 bytes respectively). + // TODO(crbug.com/646113): Make these private once DictionaryValue and + // ListValue are properly inlined. + Type type_; + union { - struct { - // TODO(crbug.com/646113): Make these private once DictionaryValue and - // ListValue are properly inlined. - Type type_ : 8; - }; - struct { - Type bool_type_ : 8; - bool bool_value_; - }; - struct { - Type int_type_ : 8; - int int_value_; - }; - struct { - Type double_type_ : 8; - // Subtle: On architectures that require it, the compiler will ensure - // that |double_value_|'s offset is a multiple of 8 (e.g. 32-bit ARM). - // See technical note above to understand why it is important. - double double_value_; - }; - struct { - Type string_type_ : 8; - std::string string_value_; - }; - struct { - Type binary_type_ : 8; - BlobStorage binary_value_; - }; - struct { - Type dict_type_ : 8; - DictStorage dict_; - }; - struct { - Type list_type_ : 8; - ListStorage list_; - }; + bool bool_value_; + int int_value_; + double double_value_; + std::string string_value_; + BlobStorage binary_value_; + DictStorage dict_; + ListStorage list_; }; private: diff --git base/values_unittest.cc base/values_unittest.cc index 2907dc066843..3a65e2d6ff2b 100644 --- base/values_unittest.cc +++ base/values_unittest.cc @@ -6,7 +6,6 @@ #include -#include #include #include #include @@ -16,7 +15,6 @@ #include #include "base/containers/adapters.h" -#include "base/logging.h" #include "base/strings/string16.h" #include "base/strings/string_piece.h" #include "base/strings/utf_string_conversions.h" @@ -26,46 +24,6 @@ namespace base { -// Test is currently incorrect on Windows x86. -#if !defined(OS_WIN) || !defined(ARCH_CPU_X86) -TEST(ValuesTest, SizeOfValue) { - // Ensure that base::Value is as small as possible, i.e. that there is - // no wasted space after the inner value due to alignment constraints. - // Distinguish between the 'header' that includes |type_| and and the inner - // value that follows it, which can be a bool, int, double, string, blob, list - // or dict. -#define INNER_TYPES_LIST(X) \ - X(bool, bool_value_) \ - X(int, int_value_) \ - X(double, double_value_) \ - X(std::string, string_value_) \ - X(Value::BlobStorage, binary_value_) \ - X(Value::ListStorage, list_) \ - X(Value::DictStorage, dict_) - -#define INNER_STRUCT_LIMIT(type, value) offsetof(Value, value) + sizeof(type), - - // Return the maximum size in bytes of each inner struct inside base::Value - size_t max_inner_struct_limit = - std::max({INNER_TYPES_LIST(INNER_STRUCT_LIMIT)}); - - // Ensure that base::Value is not larger than necessary, i.e. that there is - // no un-necessary padding afte the structs due to alignment constraints of - // one of the inner fields. - EXPECT_EQ(max_inner_struct_limit, sizeof(Value)); - if (max_inner_struct_limit != sizeof(Value)) { - // The following are useful to understand what's wrong when the EXPECT_EQ() - // above actually fails. -#define PRINT_INNER_FIELD_INFO(x, y) \ - LOG(INFO) << #y " type=" #x " size=" << sizeof(x) << " align=" << alignof(x); - - LOG(INFO) << "Value size=" << sizeof(Value) << " align=" << alignof(Value); - INNER_TYPES_LIST(PRINT_INNER_FIELD_INFO) - LOG(INFO) << "max_inner_struct_limit=" << max_inner_struct_limit; - } -} -#endif - TEST(ValuesTest, TestNothrow) { static_assert(std::is_nothrow_move_constructible::value, "IsNothrowMoveConstructible");