More efficient Value implementation. Using a union to store all possible inner data-types means less work whenever creating / copying / moving / destroying values, and small size for each instance. This results in a slightly faster "gn gen" operation, using slightly less memory, i.e. when used with a Chromium checkout: $ /usr/bin/time -f "%M %U" /tmp/aa/gn-origin gen out/Release Done. Made 21651 targets from 1614 files in 6339ms 764468 25.30 $ /usr/bin/time -f "%M %U" /tmp/aa/gn-better-value gen out/Release Done. Made 21651 targets from 1614 files in 6014ms 738132 23.27 R=brettw@chromium.org,dpranke@chromium.org BUG=NONE Change-Id: I6804857b1e69653713f038903f9abbbcae6ec62c Reviewed-on: https://gn-review.googlesource.com/c/3662 Reviewed-by: Brett Wilson <brettw@chromium.org> Commit-Queue: Brett Wilson <brettw@chromium.org>
diff --git a/tools/gn/value.cc b/tools/gn/value.cc index dc81a37..a54db03 100644 --- a/tools/gn/value.cc +++ b/tools/gn/value.cc
@@ -11,73 +11,135 @@ #include "base/strings/string_util.h" #include "tools/gn/scope.h" -Value::Value() - : type_(NONE), boolean_value_(false), int_value_(0), origin_(nullptr) {} +// NOTE: Cannot use = default here due to the use of a union member. +Value::Value() {} -Value::Value(const ParseNode* origin, Type t) - : type_(t), boolean_value_(false), int_value_(0), origin_(origin) {} +Value::Value(const ParseNode* origin, Type t) : type_(t), origin_(origin) { + switch (type_) { + case NONE: + break; + case BOOLEAN: + boolean_value_ = false; + break; + case INTEGER: + int_value_ = 0; + break; + case STRING: + new (&string_value_) std::string(); + break; + case LIST: + new (&list_value_) std::vector<Value>(); + break; + case SCOPE: + new (&scope_value_) std::unique_ptr<Scope>(); + break; + } +} Value::Value(const ParseNode* origin, bool bool_val) : type_(BOOLEAN), boolean_value_(bool_val), - int_value_(0), origin_(origin) {} Value::Value(const ParseNode* origin, int64_t int_val) : type_(INTEGER), - boolean_value_(false), int_value_(int_val), origin_(origin) {} Value::Value(const ParseNode* origin, std::string str_val) : type_(STRING), string_value_(std::move(str_val)), - boolean_value_(false), - int_value_(0), origin_(origin) {} Value::Value(const ParseNode* origin, const char* str_val) : type_(STRING), string_value_(str_val), - boolean_value_(false), - int_value_(0), origin_(origin) {} Value::Value(const ParseNode* origin, std::unique_ptr<Scope> scope) : type_(SCOPE), - string_value_(), - boolean_value_(false), - int_value_(0), scope_value_(std::move(scope)), origin_(origin) {} -Value::Value(const Value& other) - : type_(other.type_), - string_value_(other.string_value_), - boolean_value_(other.boolean_value_), - int_value_(other.int_value_), - list_value_(other.list_value_), - origin_(other.origin_) { - if (type() == SCOPE && other.scope_value_.get()) - scope_value_ = other.scope_value_->MakeClosure(); +Value::Value(const Value& other) : type_(other.type_), origin_(other.origin_) { + switch (type_) { + case NONE: + break; + case BOOLEAN: + boolean_value_ = other.boolean_value_; + break; + case INTEGER: + int_value_ = other.int_value_; + break; + case STRING: + new (&string_value_) std::string(other.string_value_); + break; + case LIST: + new (&list_value_) std::vector<Value>(other.list_value_); + break; + case SCOPE: + new (&scope_value_) std::unique_ptr<Scope>( + other.scope_value_.get() ? other.scope_value_->MakeClosure() + : nullptr); + break; + } } -Value::Value(Value&& other) noexcept = default; - -Value::~Value() = default; +Value::Value(Value&& other) noexcept + : type_(other.type_), origin_(other.origin_) { + switch (type_) { + case NONE: + break; + case BOOLEAN: + boolean_value_ = other.boolean_value_; + break; + case INTEGER: + int_value_ = other.int_value_; + break; + case STRING: + new (&string_value_) std::string(std::move(other.string_value_)); + break; + case LIST: + new (&list_value_) std::vector<Value>(std::move(other.list_value_)); + break; + case SCOPE: + new (&scope_value_) std::unique_ptr<Scope>(std::move(other.scope_value_)); + break; + } +} Value& Value::operator=(const Value& other) { - type_ = other.type_; - string_value_ = other.string_value_; - boolean_value_ = other.boolean_value_; - int_value_ = other.int_value_; - list_value_ = other.list_value_; - if (type() == SCOPE && other.scope_value_.get()) - scope_value_ = other.scope_value_->MakeClosure(); - origin_ = other.origin_; + if (this != &other) { + this->~Value(); + new (this) Value(other); + } return *this; } +Value& Value::operator=(Value&& other) noexcept { + if (this != &other) { + this->~Value(); + new (this) Value(std::move(other)); + } + return *this; +} + +Value::~Value() { + using namespace std; + switch (type_) { + case STRING: + string_value_.~string(); + break; + case LIST: + list_value_.~vector<Value>(); + break; + case SCOPE: + scope_value_.~unique_ptr<Scope>(); + break; + default:; + } +} + // static const char* Value::DescribeType(Type t) { switch (t) {
diff --git a/tools/gn/value.h b/tools/gn/value.h index 912717d..3834910 100644 --- a/tools/gn/value.h +++ b/tools/gn/value.h
@@ -47,7 +47,7 @@ ~Value(); Value& operator=(const Value& other); - Value& operator=(Value&& other) = default; + Value& operator=(Value&& other) noexcept; Type type() const { return type_; } @@ -120,18 +120,18 @@ bool operator!=(const Value& other) const; private: - // This are a lot of objects associated with every Value that need - // initialization and tear down every time. It might be more efficient to - // create a union of objects (see small_map) and only use the one we care - // about. - Type type_; - std::string string_value_; - bool boolean_value_; - int64_t int_value_; - std::vector<Value> list_value_; - std::unique_ptr<Scope> scope_value_; + void Deallocate(); - const ParseNode* origin_; + Type type_ = NONE; + const ParseNode* origin_ = nullptr; + + union { + bool boolean_value_; + int64_t int_value_; + std::string string_value_; + std::vector<Value> list_value_; + std::unique_ptr<Scope> scope_value_; + }; }; #endif // TOOLS_GN_VALUE_H_