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_