From 11436c1f0ded5e707aef2e0f10509d19c203d609 Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Sun, 7 Jan 2018 20:10:16 -0800 Subject: [PATCH] Flatten msgpack by replacing pack_array() with pack() msgpack::unpacker is not a complete streaming deserializer. It returns maps/arrays as a whole but does not allow us to step into individual elements. There is some memory overhead and it is also likely less efficient. By flattening maps, we also no longer need to track how many fields a struct has, which is less error-prone. --- src/indexer.h | 6 +-- src/language_server_api.cc | 2 +- src/language_server_api.h | 8 ++-- src/messages/text_document_hover.cc | 2 +- src/position.cc | 53 ++++++--------------------- src/query.h | 4 +- src/serializer.cc | 24 ++++++------ src/serializer.h | 57 +++++++++++++++-------------- src/serializers/json.h | 3 +- src/serializers/msgpack.h | 53 ++++++++++++++------------- 10 files changed, 96 insertions(+), 116 deletions(-) diff --git a/src/indexer.h b/src/indexer.h index f7ec4e00..13547e16 100644 --- a/src/indexer.h +++ b/src/indexer.h @@ -239,7 +239,7 @@ template void Reflect(TVisitor& visitor, TypeDefDefinitionData& value) { - REFLECT_MEMBER_START(12); + REFLECT_MEMBER_START(); REFLECT_MEMBER(short_name); REFLECT_MEMBER(detailed_name); REFLECT_MEMBER(kind); @@ -338,7 +338,7 @@ template & value) { - REFLECT_MEMBER_START(12); + REFLECT_MEMBER_START(); REFLECT_MEMBER(short_name); REFLECT_MEMBER(detailed_name); REFLECT_MEMBER(kind); @@ -452,7 +452,7 @@ template void Reflect(TVisitor& visitor, VarDefDefinitionData& value) { - REFLECT_MEMBER_START(9); + REFLECT_MEMBER_START(); REFLECT_MEMBER(short_name); REFLECT_MEMBER(detailed_name); REFLECT_MEMBER(kind); diff --git a/src/language_server_api.cc b/src/language_server_api.cc index 31940b7e..bd487ac3 100644 --- a/src/language_server_api.cc +++ b/src/language_server_api.cc @@ -194,7 +194,7 @@ void lsResponseError::Write(Writer& visitor) { auto& value = *this; int code2 = static_cast(this->code); - visitor.StartObject(2 + !!data); + visitor.StartObject(); REFLECT_MEMBER2("code", code2); REFLECT_MEMBER(message); if (data) { diff --git a/src/language_server_api.h b/src/language_server_api.h index 860c942d..4bbbdafd 100644 --- a/src/language_server_api.h +++ b/src/language_server_api.h @@ -226,7 +226,7 @@ struct lsCommand { }; template void Reflect(TVisitor& visitor, lsCommand& value) { - REFLECT_MEMBER_START(3); + REFLECT_MEMBER_START(); REFLECT_MEMBER(title); REFLECT_MEMBER(command); REFLECT_MEMBER(arguments); @@ -245,7 +245,7 @@ struct lsCodeLens { }; template void Reflect(TVisitor& visitor, lsCodeLens& value) { - REFLECT_MEMBER_START(3); + REFLECT_MEMBER_START(); REFLECT_MEMBER(range); REFLECT_MEMBER(command); REFLECT_MEMBER(data); @@ -979,7 +979,7 @@ struct Out_TextDocumentPublishDiagnostics template void Reflect(TVisitor& visitor, Out_TextDocumentPublishDiagnostics& value) { std::string method = "textDocument/publishDiagnostics"; - REFLECT_MEMBER_START(3); + REFLECT_MEMBER_START(); REFLECT_MEMBER(jsonrpc); REFLECT_MEMBER2("method", method); REFLECT_MEMBER(params); @@ -1044,7 +1044,7 @@ struct Out_ShowLogMessage : public lsOutMessage { }; template void Reflect(TVisitor& visitor, Out_ShowLogMessage& value) { - REFLECT_MEMBER_START(3); + REFLECT_MEMBER_START(); REFLECT_MEMBER(jsonrpc); std::string method = value.method(); REFLECT_MEMBER2("method", method); diff --git a/src/messages/text_document_hover.cc b/src/messages/text_document_hover.cc index 36ccc3af..61e6dc7e 100644 --- a/src/messages/text_document_hover.cc +++ b/src/messages/text_document_hover.cc @@ -58,7 +58,7 @@ struct Out_TextDocumentHover : public lsOutMessage { }; MAKE_REFLECT_STRUCT(Out_TextDocumentHover::Result, contents, range); void Reflect(Writer& visitor, Out_TextDocumentHover& value) { - REFLECT_MEMBER_START(3); + REFLECT_MEMBER_START(); REFLECT_MEMBER(jsonrpc); REFLECT_MEMBER(id); if (value.result) diff --git a/src/position.cc b/src/position.cc index 8f506857..f3514f25 100644 --- a/src/position.cc +++ b/src/position.cc @@ -66,9 +66,9 @@ bool Position::operator!=(const Position& that) const { } bool Position::operator<(const Position& that) const { - if (line < that.line) - return true; - return line == that.line && column < that.column; + if (line != that.line) + return line < that.line; + return column < that.column; } Range::Range() {} @@ -137,9 +137,9 @@ bool Range::operator!=(const Range& that) const { } bool Range::operator<(const Range& that) const { - if (start < that.start) - return true; - return start == that.start && end < that.end; + if (start != that.start) + return start < that.start; + return end < that.end; } // Position @@ -148,18 +148,8 @@ void Reflect(Reader& visitor, Position& value) { std::string s = visitor.GetString(); value = Position(s.c_str()); } else { - int i = 0; - // FIXME - static_cast(visitor).IterArray([&](Reader& entry) { - switch (i++) { - case 0: - Reflect(entry, value.line); - break; - case 1: - Reflect(entry, value.column); - break; - } - }); + Reflect(visitor, value.line); + Reflect(visitor, value.column); } } void Reflect(Writer& visitor, Position& value) { @@ -167,10 +157,8 @@ void Reflect(Writer& visitor, Position& value) { std::string output = value.ToString(); visitor.String(output.c_str(), output.size()); } else { - REFLECT_MEMBER_START(2); Reflect(visitor, value.line); Reflect(visitor, value.column); - REFLECT_MEMBER_END(); } } @@ -180,25 +168,10 @@ void Reflect(Reader& visitor, Range& value) { std::string s = visitor.GetString(); value = Range(s.c_str()); } else { - int i = 0; - // FIXME - static_cast(visitor).IterArray([&](Reader& entry) { - switch (i++) { - case 0: - Reflect(entry, value.start.line); - break; - case 1: - Reflect(entry, value.start.column); - break; - case 2: - Reflect(entry, value.end.line); - break; - case 3: - Reflect(entry, value.end.column); - break; - } - i++; - }); + Reflect(visitor, value.start.line); + Reflect(visitor, value.start.column); + Reflect(visitor, value.end.line); + Reflect(visitor, value.end.column); } } void Reflect(Writer& visitor, Range& value) { @@ -206,11 +179,9 @@ void Reflect(Writer& visitor, Range& value) { std::string output = value.ToString(); visitor.String(output.c_str(), output.size()); } else { - REFLECT_MEMBER_START(4); Reflect(visitor, value.start.line); Reflect(visitor, value.start.column); Reflect(visitor, value.end.line); Reflect(visitor, value.end.column); - REFLECT_MEMBER_END(); } } diff --git a/src/query.h b/src/query.h index 6091f990..046c5ac4 100644 --- a/src/query.h +++ b/src/query.h @@ -161,7 +161,7 @@ struct MergeableUpdate { }; template void Reflect(TVisitor& visitor, MergeableUpdate& value) { - REFLECT_MEMBER_START(3); + REFLECT_MEMBER_START(); REFLECT_MEMBER(id); REFLECT_MEMBER(to_add); REFLECT_MEMBER(to_remove); @@ -177,7 +177,7 @@ struct WithUsr { }; template void Reflect(TVisitor& visitor, WithUsr& value) { - REFLECT_MEMBER_START(2); + REFLECT_MEMBER_START(); REFLECT_MEMBER(usr); REFLECT_MEMBER(value); REFLECT_MEMBER_END(); diff --git a/src/serializer.cc b/src/serializer.cc index b6dee5ea..271bce3a 100644 --- a/src/serializer.cc +++ b/src/serializer.cc @@ -85,13 +85,13 @@ void ReflectMember(Writer& visitor, const char* name, std::string& value) { // TODO: Move this to indexer.cc void Reflect(Reader& visitor, IndexInclude& value) { - REFLECT_MEMBER_START(2); + REFLECT_MEMBER_START(); REFLECT_MEMBER(line); REFLECT_MEMBER(resolved_path); REFLECT_MEMBER_END(); } void Reflect(Writer& visitor, IndexInclude& value) { - REFLECT_MEMBER_START(2); + REFLECT_MEMBER_START(); REFLECT_MEMBER(line); if (gTestOutputMode) { std::string basename = GetBaseName(value.resolved_path); @@ -106,7 +106,7 @@ void Reflect(Writer& visitor, IndexInclude& value) { template void Reflect(TVisitor& visitor, IndexType& value) { - REFLECT_MEMBER_START(17); + REFLECT_MEMBER_START(); REFLECT_MEMBER2("id", value.id); REFLECT_MEMBER2("usr", value.usr); REFLECT_MEMBER2("short_name", value.def.short_name); @@ -129,7 +129,7 @@ void Reflect(TVisitor& visitor, IndexType& value) { template void Reflect(TVisitor& visitor, IndexFunc& value) { - REFLECT_MEMBER_START(17); + REFLECT_MEMBER_START(); REFLECT_MEMBER2("id", value.id); REFLECT_MEMBER2("is_operator", value.def.is_operator); REFLECT_MEMBER2("usr", value.usr); @@ -152,7 +152,7 @@ void Reflect(TVisitor& visitor, IndexFunc& value) { template void Reflect(TVisitor& visitor, IndexVar& value) { - REFLECT_MEMBER_START(13); + REFLECT_MEMBER_START(); REFLECT_MEMBER2("id", value.id); REFLECT_MEMBER2("usr", value.usr); REFLECT_MEMBER2("short_name", value.def.short_name); @@ -170,7 +170,7 @@ void Reflect(TVisitor& visitor, IndexVar& value) { } // IndexFile -bool ReflectMemberStart(Writer& visitor, IndexFile& value, size_t n) { +bool ReflectMemberStart(Writer& visitor, IndexFile& value) { auto it = value.id_cache.usr_to_type_id.find(""); if (it != value.id_cache.usr_to_type_id.end()) { value.Resolve(it->second)->def.short_name = ""; @@ -178,12 +178,12 @@ bool ReflectMemberStart(Writer& visitor, IndexFile& value, size_t n) { } value.version = IndexFile::kCurrentVersion; - DefaultReflectMemberStart(visitor, n); + DefaultReflectMemberStart(visitor); return true; } template void Reflect(TVisitor& visitor, IndexFile& value) { - REFLECT_MEMBER_START(5 + (gTestOutputMode ? 0 : 6)); + REFLECT_MEMBER_START(); if (!gTestOutputMode) { REFLECT_MEMBER(version); REFLECT_MEMBER(last_modification_time); @@ -257,10 +257,12 @@ std::unique_ptr Deserialize(SerializeFormat format, if (serialized.empty()) return nullptr; try { - msgpack::object_handle oh = - msgpack::unpack(serialized.data(), serialized.size()); + msgpack::unpacker upk; + upk.reserve_buffer(serialized.size()); + memcpy(upk.buffer(), serialized.data(), serialized.size()); + upk.buffer_consumed(serialized.size()); file = MakeUnique(path); - MessagePackReader reader(oh.get()); + MessagePackReader reader(&upk); Reflect(reader, *file); if (file->version != expected_version) return nullptr; diff --git a/src/serializer.h b/src/serializer.h index 0143cef6..801236af 100644 --- a/src/serializer.h +++ b/src/serializer.h @@ -23,6 +23,7 @@ class Reader { //virtual bool IsUint64() = 0; virtual bool IsString() = 0; + virtual void GetNull() = 0; virtual bool GetBool() = 0; virtual int GetInt() = 0; virtual int64_t GetInt64() = 0; @@ -50,19 +51,19 @@ class Writer { virtual void String(const char* x, size_t len) = 0; virtual void StartArray(size_t) = 0; virtual void EndArray() = 0; - virtual void StartObject(size_t) = 0; + virtual void StartObject() = 0; virtual void EndObject() = 0; virtual void Key(const char* name) = 0; }; struct IndexFile; -#define REFLECT_MEMBER_START(n) \ - if (!ReflectMemberStart(visitor, value, n)) \ - return -#define REFLECT_MEMBER_START1(value) \ - if (!ReflectMemberStart(visitor, value)) \ - return +#define REFLECT_MEMBER_START() \ + if (!ReflectMemberStart(visitor, value)) \ + return +#define REFLECT_MEMBER_START1(value) \ + if (!ReflectMemberStart(visitor, value)) \ + return #define REFLECT_MEMBER_END() ReflectMemberEnd(visitor, value); #define REFLECT_MEMBER_END1(value) ReflectMemberEnd(visitor, value); #define REFLECT_MEMBER(name) ReflectMember(visitor, #name, value.name) @@ -76,38 +77,38 @@ struct IndexFile; value = static_cast(value0); \ } -// clang-format off -// Config has many fields, we need to support at least its number of fields. -#define NUM_VA_ARGS_IMPL(_1,_2,_3,_4,_5,_6,_7,_8,_9,_10,_11,_12,_13,_14,_15,_16,_17,_18,_19,_20,_21,_22,_23,_24,_25,_26,_27,_28,_29,_30,N,...) N -#define NUM_VA_ARGS(...) NUM_VA_ARGS_IMPL(__VA_ARGS__,30,29,28,27,26,25,24,23,22,21,20,19,18,17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,1) -// clang-format on - #define _MAPPABLE_REFLECT_MEMBER(name) REFLECT_MEMBER(name); #define MAKE_REFLECT_EMPTY_STRUCT(type, ...) \ template \ void Reflect(TVisitor& visitor, type& value) { \ - REFLECT_MEMBER_START(0); \ + REFLECT_MEMBER_START(); \ REFLECT_MEMBER_END(); \ } #define MAKE_REFLECT_STRUCT(type, ...) \ template \ void Reflect(TVisitor& visitor, type& value) { \ - REFLECT_MEMBER_START(NUM_VA_ARGS(__VA_ARGS__)); \ + REFLECT_MEMBER_START(); \ MACRO_MAP(_MAPPABLE_REFLECT_MEMBER, __VA_ARGS__) \ REFLECT_MEMBER_END(); \ } +// clang-format off +// Config has many fields, we need to support at least its number of fields. +#define NUM_VA_ARGS_IMPL(_1,_2,_3,_4,_5,_6,_7,_8,_9,_10,_11,_12,_13,_14,_15,_16,_17,_18,_19,_20,_21,_22,_23,_24,_25,_26,_27,_28,_29,_30,N,...) N +#define NUM_VA_ARGS(...) NUM_VA_ARGS_IMPL(__VA_ARGS__,30,29,28,27,26,25,24,23,22,21,20,19,18,17,16,15,14,13,12,11,10,9,8,7,6,5,4,3,2,1) +// clang-format on + #define _MAPPABLE_REFLECT_ARRAY(name) Reflect(visitor, value.name); // Reflects the struct so it is serialized as an array instead of an object. // This currently only supports writers. -#define MAKE_REFLECT_STRUCT_WRITER_AS_ARRAY(type, ...) \ - inline void Reflect(Writer& visitor, type& value) { \ - visitor.StartArray(NUM_VA_ARGS(__VA_ARGS__)); \ - MACRO_MAP(_MAPPABLE_REFLECT_ARRAY, __VA_ARGS__) \ - visitor.EndArray(); \ +#define MAKE_REFLECT_STRUCT_WRITER_AS_ARRAY(type, ...) \ + inline void Reflect(Writer& visitor, type& value) { \ + visitor.StartArray(NUM_VA_ARGS(__VA_ARGS__)); \ + MACRO_MAP(_MAPPABLE_REFLECT_ARRAY, __VA_ARGS__) \ + visitor.EndArray(); \ } // API: @@ -157,8 +158,10 @@ void Reflect(Writer& visitor, std::string& value); // std::optional template void Reflect(Reader& visitor, optional& value) { - if (visitor.IsNull()) + if (visitor.IsNull()) { + visitor.GetNull(); return; + } T real_value{}; Reflect(visitor, real_value); value = real_value; @@ -201,12 +204,12 @@ void Reflect(Writer& visitor, std::vector& values) { // Writer: -inline void DefaultReflectMemberStart(Writer& visitor, size_t n) { - visitor.StartObject(n); +inline void DefaultReflectMemberStart(Writer& visitor) { + visitor.StartObject(); } template -bool ReflectMemberStart(Writer& visitor, T& value, size_t n) { - visitor.StartObject(n); +bool ReflectMemberStart(Writer& visitor, T& value) { + visitor.StartObject(); return true; } template @@ -238,9 +241,9 @@ void ReflectMember(Writer& visitor, const char* name, std::string& value); // Reader: -inline void DefaultReflectMemberStart(Reader& visitor, size_t n) {} +inline void DefaultReflectMemberStart(Reader& visitor) {} template -bool ReflectMemberStart(Reader& visitor, T& value, size_t n) { +bool ReflectMemberStart(Reader& visitor, T& value) { return true; } template diff --git a/src/serializers/json.h b/src/serializers/json.h index 8cb92bc8..38b2cf65 100644 --- a/src/serializers/json.h +++ b/src/serializers/json.h @@ -20,6 +20,7 @@ class JsonReader : public Reader { //bool IsUint64() override { return m_->IsUint64(); } bool IsString() override { return m_->IsString(); } + void GetNull() override {} bool GetBool() override { return m_->GetBool(); } int GetInt() override { return m_->GetInt(); } int64_t GetInt64() override { return m_->GetInt64(); } @@ -67,7 +68,7 @@ class JsonWriter : public Writer { void String(const char* x, size_t len) override { m_->String(x, len); } void StartArray(size_t) override { m_->StartArray(); } void EndArray() override { m_->EndArray(); } - void StartObject(size_t) override { m_->StartObject(); } + void StartObject() override { m_->StartObject(); } void EndObject() override { m_->EndObject(); } void Key(const char* name) override { m_->Key(name); } }; diff --git a/src/serializers/msgpack.h b/src/serializers/msgpack.h index c2468e8a..9d5a57e2 100644 --- a/src/serializers/msgpack.h +++ b/src/serializers/msgpack.h @@ -5,27 +5,34 @@ #include class MessagePackReader : public Reader { - msgpack::object o_; - size_t idx_ = 0; - std::vector children_; + msgpack::unpacker* pk_; + msgpack::object_handle oh_; + + template + T Get() { + T ret = oh_.get().as(); + pk_->next(oh_); + return ret; + } public: - MessagePackReader(msgpack::object o) : o_(o) {} + MessagePackReader(msgpack::unpacker* pk) : pk_(pk) { pk->next(oh_); } SerializeFormat Format() const override { return SerializeFormat::MessagePack; } - bool IsNull() override { return o_.is_nil(); } - bool IsArray() override { return o_.type == msgpack::type::ARRAY; } + bool IsNull() override { return oh_.get().is_nil(); } + bool IsArray() override { return oh_.get().type == msgpack::type::ARRAY; } bool IsInt() override { - return o_.type == msgpack::type::POSITIVE_INTEGER || - o_.type == msgpack::type::NEGATIVE_INTEGER; + return oh_.get().type == msgpack::type::POSITIVE_INTEGER || + oh_.get().type == msgpack::type::NEGATIVE_INTEGER; } - bool IsString() override { return o_.type == msgpack::type::STR; } + bool IsString() override { return oh_.get().type == msgpack::type::STR; } - bool GetBool() override { return o_.as(); } - int GetInt() override { return o_.as(); } - int64_t GetInt64() override { return o_.as(); } - uint64_t GetUint64() override { return o_.as(); } - std::string GetString() override { return o_.as(); } + void GetNull() override { pk_->next(oh_); } + bool GetBool() override { return Get(); } + int GetInt() override { return Get(); } + int64_t GetInt64() override { return Get(); } + uint64_t GetUint64() override { return Get(); } + std::string GetString() override { return Get(); } bool HasMember(const char* x) override { return true; } std::unique_ptr operator[](const char* x) override { @@ -33,17 +40,13 @@ class MessagePackReader : public Reader { } void IterArray(std::function fn) override { - for (auto& entry : o_.as>()) { - MessagePackReader sub(entry); - fn(sub); - } + size_t n = Get(); + for (size_t i = 0; i < n; i++) + fn(*this); } - void DoMember(const char* name, std::function fn) override { - if (idx_ == 0) - children_ = o_.as>(); - MessagePackReader sub(children_[idx_++]); - fn(sub); + void DoMember(const char*, std::function fn) override { + fn(*this); } }; @@ -62,9 +65,9 @@ class MessagePackWriter : public Writer { void String(const char* x) override { m_->pack(x); } // TODO Remove std::string void String(const char* x, size_t len) override { m_->pack(std::string(x, len)); } - void StartArray(size_t n) override { m_->pack_array(uint32_t(n)); } + void StartArray(size_t n) override { m_->pack(n); } void EndArray() override {} - void StartObject(size_t n) override { m_->pack_array(uint32_t(n)); } + void StartObject() override {} void EndObject() override {} void Key(const char* name) override {} };