Bug 1594322 - Part 4: Add better accessors to HuffmanDictionary. r=Yoric
authorTooru Fujisawa <arai_a@mac.com>
Fri, 22 Nov 2019 14:38:03 +0000
changeset 503363 fd82074eab1d8d4642d816436c166267350ecf6f
parent 503362 f395a7657d621a2d9463efca340a4b319da59eb4
child 503364 ba7c82709e9f28a58a21cbc699cd89beeb804d28
push id36833
push userbtara@mozilla.com
push dateFri, 22 Nov 2019 21:40:53 +0000
treeherdermozilla-central@2c912e46295e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersYoric
bugs1594322
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1594322 - Part 4: Add better accessors to HuffmanDictionary. r=Yoric Hide the internal of status and table creation/access from consumers. Depends on D52032 Differential Revision: https://phabricator.services.mozilla.com/D52034
js/src/frontend/BinASTTokenReaderContext.cpp
js/src/frontend/BinASTTokenReaderContext.h
--- a/js/src/frontend/BinASTTokenReaderContext.cpp
+++ b/js/src/frontend/BinASTTokenReaderContext.cpp
@@ -465,31 +465,30 @@ class HuffmanPreludeReader {
   interface’s fields
   5. Otherwise, push the field onto the stack
   */
 
   // Enqueue an entry to the stack.
   MOZ_MUST_USE JS::Result<Ok> pushValue(NormalizedInterfaceAndField identity,
                                         const List& list) {
     const auto tableId = HuffmanDictionary::TableIdentity(list.contents_);
-    auto& status = dictionary_.status(tableId);
-    if (status == HuffmanDictionary::TableStatus::Unreachable) {
+    if (dictionary_.isUnreachable(tableId)) {
       // Spec:
       // 2. Add the field to the set of visited fields
-      status = HuffmanDictionary::TableStatus::Initializing;
+      dictionary_.setInitializing(tableId);
 
       // Read the lengths immediately.
       MOZ_TRY((readTable<List>(tableId, list)));
     }
 
     // Spec:
     // 3. If the field has a FrozenArray type
     //   a. Determine if the array type is always empty
     //   b. If so, stop
-    auto& table = dictionary_.table(tableId);
+    const auto& table = dictionary_.getTable(tableId);
     bool empty = true;
     for (auto iter : table) {
       if (iter->toListLength() > 0) {
         empty = false;
         break;
       }
     }
     if (empty) {
@@ -517,23 +516,19 @@ class HuffmanPreludeReader {
 #undef WRAP_LIST_2
     }
     return Ok();
   }
 
   MOZ_MUST_USE JS::Result<Ok> pushValue(NormalizedInterfaceAndField identity,
                                         const Interface& interface) {
     const auto tableId = HuffmanDictionary::TableIdentity(identity);
-    auto& status = dictionary_.status(tableId);
-    if (status == HuffmanDictionary::TableStatus::Unreachable) {
+    if (dictionary_.isUnreachable(tableId)) {
       // Effectively, an `Interface` is a sum with a single entry.
-      auto& table = dictionary_.table(tableId);
-
-      status = HuffmanDictionary::TableStatus::Ready;
-      new (mozilla::KnownNotNull, &table) GenericHuffmanTable();
+      auto& table = dictionary_.createTable(tableId);
 
       MOZ_TRY(table.initWithSingleValue(
           cx_, BinASTSymbol::fromKind(BinASTKind(interface.kind_))));
     }
 
     // Spec:
     // 4. If the effective type is a monomorphic interface, push all of the
     // interface’s fields
@@ -542,25 +537,24 @@ class HuffmanPreludeReader {
 
   // Generic implementation for other cases.
   template <class Entry>
   MOZ_MUST_USE JS::Result<Ok> pushValue(NormalizedInterfaceAndField identity,
                                         const Entry& entry) {
     // Spec:
     // 1. If the field is in the set of visited contexts, stop.
     const auto tableId = HuffmanDictionary::TableIdentity(identity);
-    auto& status = dictionary_.status(tableId);
-    if (status != HuffmanDictionary::TableStatus::Unreachable) {
+    if (!dictionary_.isUnreachable(tableId)) {
       // Entry already initialized/initializing.
       return Ok();
     }
 
     // Spec:
     // 2. Add the field to the set of visited fields
-    status = HuffmanDictionary::TableStatus::Initializing;
+    dictionary_.setInitializing(tableId);
 
     // Spec:
     // 5. Otherwise, push the field onto the stack
     BINJS_TRY(stack_.append(entry));
     return Ok();
   }
 
   // Enqueue all the fields of an interface to the stack.
@@ -819,18 +813,17 @@ class HuffmanPreludeReader {
     return Ok();
   }
 
   // Single-argument version: lookup the table using `dictionary_.table`
   // then proceed as three-arguments version.
   template <typename Entry>
   MOZ_MUST_USE JS::Result<Ok> readTable(Entry entry) {
     const auto tableId = HuffmanDictionary::TableIdentity(entry.identity_);
-    auto& status = dictionary_.status(tableId);
-    if (MOZ_UNLIKELY(status != HuffmanDictionary::TableStatus::Initializing)) {
+    if (MOZ_UNLIKELY(!dictionary_.isInitializing(tableId))) {
       // We're attempting to re-read a table that has already been read.
       // FIXME: Shouldn't this be a MOZ_CRASH?
       return raiseDuplicateTableError(entry.identity_);
     }
 
     return readTable<Entry>(tableId, entry);
   }
 
@@ -839,32 +832,24 @@ class HuffmanPreludeReader {
   // lengths.
   template <typename Entry>
   MOZ_MUST_USE JS::Result<Ok> readTable(
       HuffmanDictionary::TableIdentity tableId, Entry entry) {
     uint8_t headerByte;
     MOZ_TRY_VAR(headerByte, reader_.readByte<Compression::No>());
     switch (headerByte) {
       case TableHeader::SingleValue: {
-        auto& table = dictionary_.table(tableId);
-        auto& status = dictionary_.status(tableId);
-
-        new (mozilla::KnownNotNull, &table) GenericHuffmanTable();
-        status = HuffmanDictionary::TableStatus::Ready;
+        auto& table = dictionary_.createTable(tableId);
 
         // The table contains a single value.
         MOZ_TRY((readSingleValueTable<Entry>(table, entry)));
         return Ok();
       }
       case TableHeader::MultipleValues: {
-        auto& table = dictionary_.table(tableId);
-        auto& status = dictionary_.status(tableId);
-
-        new (mozilla::KnownNotNull, &table) GenericHuffmanTable();
-        status = HuffmanDictionary::TableStatus::Ready;
+        auto& table = dictionary_.createTable(tableId);
 
         // Table contains multiple values.
         MOZ_TRY((readMultipleValuesTable<Entry>(table, entry)));
         return Ok();
       }
       case TableHeader::Unreachable:
         // Table is unreachable, nothing to do.
         return Ok();
@@ -923,22 +908,21 @@ class HuffmanPreludeReader {
       // First, we need a table to determine whether the value is `null`.
       MOZ_TRY((owner.readTable<MaybeInterface>(entry)));
 
       // Then, if the table contains `true`, we need the fields of the
       // interface.
       // FIXME: readTable could return a reference to the table, eliminating an
       // array lookup.
       const auto tableId = HuffmanDictionary::TableIdentity(entry.identity_);
-      auto& status = owner.dictionary_.status(tableId);
-      if (status == HuffmanDictionary::TableStatus::Unreachable) {
+      if (owner.dictionary_.isUnreachable(tableId)) {
         return Ok();
       }
 
-      const auto& table = owner.dictionary_.table(tableId);
+      const auto& table = owner.dictionary_.getTable(tableId);
       if (!table.isMaybeInterfaceAlwaysNull()) {
         MOZ_TRY(owner.pushFields(entry.kind_));
       }
       return Ok();
     }
 
     // Sum of interfaces, non-nullable.
     // Values: interfaces by the order in `FOR_EACH_BIN_INTERFACE_IN_SUM_*`.
@@ -946,23 +930,22 @@ class HuffmanPreludeReader {
       // First, we need a table to determine which values are present.
       MOZ_TRY((owner.readTable<Sum>(entry)));
 
       // Now, walk the table to enqueue each value if necessary.
       // FIXME: readTable could return a reference to the table, eliminating an
       // array lookup.
 
       const auto tableId = HuffmanDictionary::TableIdentity(entry.identity_);
-      auto& status = owner.dictionary_.status(tableId);
-      if (status == HuffmanDictionary::TableStatus::Initializing) {
+      if (owner.dictionary_.isInitializing(tableId)) {
         return Ok();
       }
 
       auto index = entry.identity_;
-      const auto& table = owner.dictionary_.table(tableId);
+      const auto& table = owner.dictionary_.getTable(tableId);
       for (auto iter : table) {
         MOZ_TRY(owner.pushValue(index, Interface(index, iter->toKind())));
       }
       return Ok();
     }
 
     // Sum of interfaces, nullable.
     // Values: `null`, followed by interfaces by the order in
@@ -970,23 +953,22 @@ class HuffmanPreludeReader {
     MOZ_MUST_USE JS::Result<Ok> operator()(const MaybeSum& entry) {
       // First, we need a table to determine which values are present.
       MOZ_TRY((owner.readTable<MaybeSum>(entry)));
 
       // Now, walk the table to enqueue each value if necessary.
       // FIXME: readTable could return a reference to the table, eliminating an
       // array lookup.
       const auto tableId = HuffmanDictionary::TableIdentity(entry.identity_);
-      auto& status = owner.dictionary_.status(tableId);
-      if (status == HuffmanDictionary::TableStatus::Unreachable) {
+      if (owner.dictionary_.isUnreachable(tableId)) {
         return Ok();
       }
 
       auto index = entry.identity_;
-      const auto& table = owner.dictionary_.table(tableId);
+      const auto& table = owner.dictionary_.getTable(tableId);
       for (auto iter : table) {
         MOZ_TRY(owner.pushValue(index, Interface(index, iter->toKind())));
       }
       return Ok();
     }
 
     MOZ_MUST_USE JS::Result<Ok> operator()(const Number& entry) {
       return owner.readTable<Number>(entry);
@@ -1340,39 +1322,38 @@ struct ExtractBinASTInterfaceAndFieldMat
   }
 };
 
 JS::Result<BinASTKind> BinASTTokenReaderContext::readTagFromTable(
     const BinASTInterfaceAndField& identity) {
   // Extract the table.
   const auto tableId =
       HuffmanDictionary::TableIdentity(NormalizedInterfaceAndField(identity));
-  const auto& table = dictionary_.table(tableId);
+  const auto& table = dictionary_.getTable(tableId);
   BINJS_MOZ_TRY_DECL(bits_,
                      (bitBuffer.getHuffmanLookup<Compression::No>(*this)));
 
   // We're entering either a single interface or a sum.
   const auto result = table.lookup(bits_);
   if (MOZ_UNLIKELY(!result.isFound())) {
     return raiseInvalidValue();
   }
   bitBuffer.advanceBitBuffer<Compression::No>(result.bitLength());
   return result.value().toKind();
 }
 
 JS::Result<BinASTSymbol> BinASTTokenReaderContext::readFieldFromTable(
     const BinASTInterfaceAndField& identity) {
   const auto tableId =
       HuffmanDictionary::TableIdentity(NormalizedInterfaceAndField(identity));
-  auto& status = dictionary_.status(tableId);
-  if (status != HuffmanDictionary::TableStatus::Ready) {
+  if (!dictionary_.isReady(tableId)) {
     return raiseNotInPrelude();
   }
 
-  const auto& table = dictionary_.table(tableId);
+  const auto& table = dictionary_.getTable(tableId);
   BINJS_MOZ_TRY_DECL(bits_, bitBuffer.getHuffmanLookup<Compression::No>(*this));
 
   const auto result = table.lookup(bits_);
   if (MOZ_UNLIKELY(!result.isFound())) {
     return raiseInvalidValue();
   }
 
   bitBuffer.advanceBitBuffer<Compression::No>(result.bitLength());
@@ -1502,17 +1483,17 @@ JS::Result<Ok> BinASTTokenReaderContext:
   // This tuple is the value of the field we're currently reading.
   MOZ_TRY_VAR(tag, readTagFromTable(context.position_));
   return Ok();
 }
 
 JS::Result<Ok> BinASTTokenReaderContext::enterList(uint32_t& items,
                                                    const ListContext& context) {
   const auto tableId = HuffmanDictionary::TableIdentity(context.content_);
-  const auto& table = dictionary_.table(tableId);
+  const auto& table = dictionary_.getTable(tableId);
   BINJS_MOZ_TRY_DECL(bits_, bitBuffer.getHuffmanLookup<Compression::No>(*this));
   const auto result = table.lookup(bits_);
   if (MOZ_UNLIKELY(!result.isFound())) {
     return raiseInvalidValue();
   }
   bitBuffer.advanceBitBuffer<Compression::No>(result.bitLength());
   items = result.value().toListLength();
   return Ok();
@@ -2804,17 +2785,17 @@ HuffmanPreludeReader::readSingleValueTab
   BINJS_MOZ_TRY_DECL(index, reader_.readUnpackedLong());
   MOZ_TRY(
       table.initWithSingleValue(cx_, BinASTSymbol::fromUnsignedLong(index)));
   return Ok();
 }
 
 HuffmanDictionary::~HuffmanDictionary() {
   for (size_t i = 0; i < TableIdentity::Limit; i++) {
-    if (status(i) == TableStatus::Ready) {
+    if (status_[i] == TableStatus::Ready) {
       table(i).~GenericHuffmanTable();
     }
   }
 }
 
 uint32_t HuffmanLookup::leadingBits(const uint8_t aBitLength) const {
   MOZ_ASSERT(aBitLength <= bitLength_);
   const uint32_t result = (aBitLength == 0)
--- a/js/src/frontend/BinASTTokenReaderContext.h
+++ b/js/src/frontend/BinASTTokenReaderContext.h
@@ -185,44 +185,36 @@ class alignas(8) BinASTSymbol {
     return fromRawBits(mozilla::BitwiseCast<uint64_t>(d));
   }
   static BinASTSymbol fromKind(BinASTKind k) {
     return fromRawBits(uint64_t(k));
   }
   static BinASTSymbol fromVariant(BinASTVariant v) {
     return fromRawBits(uint64_t(v));
   }
-  static BinASTSymbol fromAtomIndex(size_t i) {
-    return fromRawBits(i);
-  }
-  static BinASTSymbol nullAtom() {
-    return fromRawBits(NullAtomIndex);
-  }
+  static BinASTSymbol fromAtomIndex(size_t i) { return fromRawBits(i); }
+  static BinASTSymbol nullAtom() { return fromRawBits(NullAtomIndex); }
 
   uint32_t toUnsignedLong() const { return uint32_t(asBits_); }
   uint32_t toListLength() const { return uint32_t(asBits_); }
   size_t toSubtableIndex() const { return size_t(asBits_); }
   bool toBool() const { return bool(asBits_); }
   double toDouble() const { return mozilla::BitwiseCast<double>(asBits_); }
   BinASTKind toKind() const { return BinASTKind(asBits_); }
   BinASTVariant toVariant() const { return BinASTVariant(asBits_); }
 
   size_t toAtomIndex() const {
     MOZ_ASSERT(!isNullAtom());
     return toAtomIndexNoCheck();
   }
 
-  bool isNullAtom() const {
-    return toAtomIndexNoCheck() == NullAtomIndex;
-  }
+  bool isNullAtom() const { return toAtomIndexNoCheck() == NullAtomIndex; }
 
-  private:
-  size_t toAtomIndexNoCheck() const {
-    return size_t(asBits_);
-  }
+ private:
+  size_t toAtomIndexNoCheck() const { return size_t(asBits_); }
 };
 
 // An entry in a Huffman table.
 class HuffmanEntry {
   const HuffmanKey key_;
   const BinASTSymbol value_;
 
  public:
@@ -885,77 +877,98 @@ struct GenericHuffmanTable {
 };
 
 // A Huffman dictionary for the current file.
 //
 // A Huffman dictionary consists in a (contiguous) set of Huffman tables
 // to predict field values and a second (contiguous) set of Huffman tables
 // to predict list lengths.
 class HuffmanDictionary {
- public:
-  HuffmanDictionary() {}
-  ~HuffmanDictionary();
-
   // While reading the Huffman prelude, whenever we first encounter a
   // table with `Unreachable` status, we set its status with a `Initializing`
   // to mark that we should not attempt to read/initialize it again.
   // Once the table is initialized, it becomes `Ready`.
   enum class TableStatus : uint8_t {
     Unreachable,
     Initializing,
     Ready,
   };
 
+ public:
+  HuffmanDictionary() {}
+  ~HuffmanDictionary();
+
   // Handles the mapping from NormalizedInterfaceAndField and BinASTList to
   // the index inside the list of huffman tables.
   class TableIdentity {
     size_t index_;
 
     static const size_t ListIdentityBase = BINAST_INTERFACE_AND_FIELD_LIMIT;
 
    public:
     // The maximum number of tables.
     static const size_t Limit = ListIdentityBase + BINAST_NUMBER_OF_LIST_TYPES;
 
     explicit TableIdentity(NormalizedInterfaceAndField index)
         : index_(static_cast<size_t>(index.identity_)) {}
     explicit TableIdentity(BinASTList list)
         : index_(static_cast<size_t>(list) + ListIdentityBase) {}
 
-    size_t toIndex() const {
-      return index_;
-    }
+    size_t toIndex() const { return index_; }
   };
 
-  TableStatus& status(TableIdentity i) {
-    return status(i.toIndex());
+  bool isUnreachable(TableIdentity i) const {
+    return status_[i.toIndex()] == TableStatus::Unreachable;
+  }
+
+  bool isInitializing(TableIdentity i) const {
+    return status_[i.toIndex()] == TableStatus::Initializing;
+  }
+
+  bool isReady(TableIdentity i) const {
+    return status_[i.toIndex()] == TableStatus::Ready;
+  }
+
+  void setInitializing(TableIdentity i) {
+    status_[i.toIndex()] = TableStatus::Initializing;
   }
 
-  GenericHuffmanTable& table(TableIdentity i) {
+ private:
+  void setReady(TableIdentity i) { status_[i.toIndex()] = TableStatus::Ready; }
+
+ public:
+  GenericHuffmanTable& createTable(TableIdentity i) {
+    MOZ_ASSERT(isUnreachable(i) || isInitializing(i));
+
+    setReady(i);
+
+    auto& t = table(i.toIndex());
+    new (mozilla::KnownNotNull, &t) GenericHuffmanTable();
+
+    return t;
+  }
+
+  const GenericHuffmanTable& getTable(TableIdentity i) const {
+    MOZ_ASSERT(isReady(i));
     return table(i.toIndex());
   }
 
  private:
   // For the following purpose, tables are stored as an array of status
   // and a uninitialized buffer to store an array of tables.
   //
   //   * In most case a single BinAST file doesn't use all tables
   //   * GenericHuffmanTable constructor/destructor costs are not negligible,
   //     and we don't want to call them for unused tables
   //   * Initializing status for whether the table is used or not takes
   //     less time if they're stored in contiguous memory, instead of
   //     placed before each table (using `Variant` or `Maybe`)
   //
   // Tables with `Ready` status are destructed in HuffmanDictionary destructor.
-  TableStatus status_[TableIdentity::Limit] = {
-      TableStatus::Unreachable};
-
-  TableStatus& status(size_t i) {
-    return status_[i];
-  }
+  TableStatus status_[TableIdentity::Limit] = {TableStatus::Unreachable};
 
   // Huffman tables for either:
   //   * `(Interface, Field)` pairs, used to decode the value of
   //     `Interface::Field`.
   //   * list lengths
   // Some entries may be uninitialized if they represent `(Interface, Field)`
   // pairs or lists that actually do not show up in the file.
   //
@@ -965,16 +978,20 @@ class HuffmanDictionary {
   // Semantically this is `GenericHuffmanTable tables_[TableIdentity::Limit]`,
   // but items are constructed lazily.
   alignas(GenericHuffmanTable) char tables_[sizeof(GenericHuffmanTable) *
                                             TableIdentity::Limit];
 
   GenericHuffmanTable& table(size_t i) {
     return (reinterpret_cast<GenericHuffmanTable*>(tables_))[i];
   }
+
+  const GenericHuffmanTable& table(size_t i) const {
+    return (reinterpret_cast<const GenericHuffmanTable*>(tables_))[i];
+  }
 };
 
 /**
  * A token reader implementing the "context" serialization format for BinAST.
  *
  * This serialization format, which is also supported by the reference
  * implementation of the BinAST compression suite, is designed to be
  * space- and time-efficient.