Refactor ScriptData class for cached compile data.

src/parser.cc

Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index 5b306a7035efc3adce275a201a4f6edd0220894f..8110dece589ca3b0b425921bcb00889eec538794 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -182,60 +182,42 @@ void RegExpBuilder::AddQuantifierToAtom(
 }
 
 
-ScriptData* ScriptData::New(const char* data, int length, bool owns_store) {
-  // The length is obviously invalid.
-  if (length % sizeof(unsigned) != 0) {
-    return NULL;
-  }
-
-  int deserialized_data_length = length / sizeof(unsigned);
-  unsigned* deserialized_data;
-  owns_store =
-      owns_store || reinterpret_cast<intptr_t>(data) % sizeof(unsigned) != 0;
-  if (owns_store) {
-    // Copy the data to align it.
-    deserialized_data = i::NewArray<unsigned>(deserialized_data_length);
-    i::CopyBytes(reinterpret_cast<char*>(deserialized_data),
-                 data, static_cast<size_t>(length));
-  } else {
-    // If aligned, don't create a copy of the data.
-    deserialized_data = reinterpret_cast<unsigned*>(const_cast<char*>(data));
-  }
-  return new ScriptData(
-      Vector<unsigned>(deserialized_data, deserialized_data_length),
-      owns_store);
-}
-
-
-FunctionEntry ScriptData::GetFunctionEntry(int start) {
+FunctionEntry ParseData::GetFunctionEntry(int start) {
   // The current pre-data entry must be a FunctionEntry with the given
   // start position.
-  if ((function_index_ + FunctionEntry::kSize <= store_.length())
-      && (static_cast<int>(store_[function_index_]) == start)) {
+  if ((function_index_ + FunctionEntry::kSize <= Length()) &&
+      (static_cast<int>(Data()[function_index_]) == start)) {
     int index = function_index_;
     function_index_ += FunctionEntry::kSize;
-    return FunctionEntry(store_.SubVector(index,
-                                          index + FunctionEntry::kSize));
+    Vector<unsigned> subvector(&(Data()[index]), FunctionEntry::kSize);
+    return FunctionEntry(subvector);
   }
   return FunctionEntry();
 }
 
 
-int ScriptData::GetSymbolIdentifier() {
-  return ReadNumber(&symbol_data_);
+int ParseData::GetSymbolIdentifier() { return ReadNumber(&symbol_data_); }
+
+
+int ParseData::FunctionCount() {
+  int functions_size = FunctionsSize();
+  if (functions_size < 0) return 0;
+  if (functions_size % FunctionEntry::kSize != 0) return 0;
+  return functions_size / FunctionEntry::kSize;
 }
 
 
-bool ScriptData::SanityCheck() {
+bool ParseData::Sanity() {
   // Check that the header data is valid and doesn't specify
   // point to positions outside the store.
-  if (store_.length() < PreparseDataConstants::kHeaderSize) return false;
-  if (magic() != PreparseDataConstants::kMagicNumber) return false;
-  if (version() != PreparseDataConstants::kCurrentVersion) return false;
-  if (has_error()) {
+  int data_length = Length();
+  if (data_length < PreparseDataConstants::kHeaderSize) return false;
+  if (Magic() != PreparseDataConstants::kMagicNumber) return false;
+  if (Version() != PreparseDataConstants::kCurrentVersion) return false;
+  if (HasError()) {

[marja, 2014/07/09 14:42:30]

Afaics we'll never have an error in normal operation, just in tests. So maybe
you could just return false here?

[Yang, 2014/07/10 08:28:46]

Done.

But if that's the case, couldn't we just remove that HasError field entirely?

     // Extra sane sanity check for error message encoding.
-    if (store_.length() <= PreparseDataConstants::kHeaderSize
-                         + PreparseDataConstants::kMessageTextPos) {
+    if (data_length <= PreparseDataConstants::kHeaderSize +
+                           PreparseDataConstants::kMessageTextPos) {
       return false;
     }
     if (Read(PreparseDataConstants::kMessageStartPos) >
@@ -245,63 +227,95 @@ bool ScriptData::SanityCheck() {
     unsigned arg_count = Read(PreparseDataConstants::kMessageArgCountPos);
     int pos = PreparseDataConstants::kMessageTextPos;
     for (unsigned int i = 0; i <= arg_count; i++) {
-      if (store_.length() <= PreparseDataConstants::kHeaderSize + pos) {
+      if (data_length <= PreparseDataConstants::kHeaderSize + pos) {
         return false;
       }
       int length = static_cast<int>(Read(pos));
       if (length < 0) return false;
       pos += 1 + length;
     }
-    if (store_.length() < PreparseDataConstants::kHeaderSize + pos) {
+    if (data_length < PreparseDataConstants::kHeaderSize + pos) {
       return false;
     }
     return true;
   }
   // Check that the space allocated for function entries is sane.
-  int functions_size =
-      static_cast<int>(store_[PreparseDataConstants::kFunctionsSizeOffset]);
+  int functions_size = FunctionsSize();
   if (functions_size < 0) return false;
   if (functions_size % FunctionEntry::kSize != 0) return false;
   // Check that the total size has room for header and function entries.
   int minimum_size =
       PreparseDataConstants::kHeaderSize + functions_size;
-  if (store_.length() < minimum_size) return false;
+  if (data_length < minimum_size) return false;
   return true;
 }
 
 
+void ParseData::Initialize() {
+  // Prepares state for use.
+  int data_length = Length();
+  if (data_length >= PreparseDataConstants::kHeaderSize) {
+    function_index_ = PreparseDataConstants::kHeaderSize;
+    int symbol_data_offset =

[marja, 2014/07/09 14:42:30]

Omg, this is messier than I thought. We don't have symbol data any more! Could
you remove it since you're at it?

[Yang, 2014/07/10 08:28:46]

Done.

+        PreparseDataConstants::kHeaderSize + FunctionsSize();
+    unsigned* data = Data();
+    if (data_length > symbol_data_offset) {
+      symbol_data_ = reinterpret_cast<byte*>(data[symbol_data_offset]);
+    } else {
+      // Partial preparse causes no symbol information.
+      symbol_data_ = reinterpret_cast<byte*>(data + data_length);
+    }

[vogelheim, 2014/07/09 17:23:36]

nitpick, and not your code, but... this duplicates a bit of logic, which took me
a while to figure out. I'd have written it something like this:

int offset = (data_length > symbol_data_offset)
    ? symbol_data_offset : data_length;
unsigned* data = ...
symbol_data_ = reinterpret_cast<byte*>(data[offset]);
symbol_data_end_ = reinterpret_cast<byte*>(data[data_length]);

[Yang, 2014/07/10 08:28:45]

All gone now anyways.

+    symbol_data_end_ = reinterpret_cast<byte*>(data + data_length);
+  }
+}
 
-const char* ScriptData::ReadString(unsigned* start, int* chars) {
-  int length = start[0];
-  char* result = NewArray<char>(length + 1);
-  for (int i = 0; i < length; i++) {
-    result[i] = start[i + 1];
+
+int ParseData::ReadNumber(byte** source) {
+  // Reads a number from symbol_data_ in base 128. The most significant
+  // bit marks that there are more digits.
+  // If the first byte is 0x80 (kNumberTerminator), it would normally
+  // represent a leading zero. Since that is useless, and therefore won't
+  // appear as the first digit of any actual value, it is used to
+  // mark the end of the input stream.
+  byte* data = *source;
+  if (data >= symbol_data_end_) return -1;
+  byte input = *data;
+  if (input == PreparseDataConstants::kNumberTerminator) {
+    // End of stream marker.
+    return -1;
   }
-  result[length] = '\0';
-  if (chars != NULL) *chars = length;
+  int result = input & 0x7f;
+  data++;
+  while ((input & 0x80u) != 0) {
+    if (data >= symbol_data_end_) return -1;
+    input = *data;
+    result = (result << 7) | (input & 0x7f);
+    data++;
+  }
+  *source = data;
   return result;
 }
 
 
-Scanner::Location ScriptData::MessageLocation() const {
+Scanner::Location ParseData::MessageLocation() const {
   int beg_pos = Read(PreparseDataConstants::kMessageStartPos);
   int end_pos = Read(PreparseDataConstants::kMessageEndPos);
   return Scanner::Location(beg_pos, end_pos);
 }
 
 
-bool ScriptData::IsReferenceError() const {
+bool ParseData::IsReferenceError() const {
   return Read(PreparseDataConstants::kIsReferenceErrorPos);
 }
 
 
-const char* ScriptData::BuildMessage() const {
+const char* ParseData::BuildMessage() {
   unsigned* start = ReadAddress(PreparseDataConstants::kMessageTextPos);
   return ReadString(start, NULL);
 }
 
 
-const char* ScriptData::BuildArg() const {
+const char* ParseData::BuildArg() {
   int arg_count = Read(PreparseDataConstants::kMessageArgCountPos);
   ASSERT(arg_count == 0 || arg_count == 1);
   if (arg_count == 0) {
@@ -316,13 +330,52 @@ const char* ScriptData::BuildArg() const {
 }
 
 
-unsigned ScriptData::Read(int position) const {
-  return store_[PreparseDataConstants::kHeaderSize + position];
+const char* ParseData::ReadString(unsigned* start, int* chars) {
+  int length = start[0];
+  char* result = NewArray<char>(length + 1);
+  for (int i = 0; i < length; i++) {
+    result[i] = start[i + 1];
+  }
+  result[length] = '\0';
+  if (chars != NULL) *chars = length;
+  return result;
+}
+
+
+bool ParseData::HasError() const {
+  return Data()[PreparseDataConstants::kHasErrorOffset];
+}
+
+unsigned ParseData::Magic() const {
+  return Data()[PreparseDataConstants::kMagicOffset];
+}
+
+unsigned ParseData::Version() const {
+  return Data()[PreparseDataConstants::kVersionOffset];
+}
+
+int ParseData::FunctionsSize() const {
+  return static_cast<int>(Data()[PreparseDataConstants::kFunctionsSizeOffset]);
+}
+
+unsigned ParseData::Read(int position) const {
+  return Data()[PreparseDataConstants::kHeaderSize + position];
+}
+
+unsigned* ParseData::ReadAddress(int position) const {
+  return &(Data()[PreparseDataConstants::kHeaderSize + position]);
 }
 
 
-unsigned* ScriptData::ReadAddress(int position) const {
-  return &store_[PreparseDataConstants::kHeaderSize + position];
+void Parser::SetCachedData() {
+  if (cached_data_mode() == NO_CACHED_DATA) {
+    cached_parse_data_ = NULL;
+  } else {
+    ASSERT(info_->cached_data() != NULL);
+    if (cached_data_mode() == CONSUME_CACHED_DATA) {
+      cached_parse_data_ = new ParseData(*info_->cached_data());
+    }
+  }
 }
 
 
@@ -757,18 +810,14 @@ FunctionLiteral* ParserTraits::ParseFunctionLiteral(
 Parser::Parser(CompilationInfo* info)
     : ParserBase<ParserTraits>(&scanner_,
                                info->isolate()->stack_guard()->real_climit(),
-                               info->extension(),
-                               NULL,
-                               info->zone(),
-                               this),
+                               info->extension(), NULL, info->zone(), this),
       isolate_(info->isolate()),
       script_(info->script()),
       scanner_(isolate_->unicode_cache()),
       reusable_preparser_(NULL),
       original_scope_(NULL),
       target_stack_(NULL),
-      cached_data_(NULL),
-      cached_data_mode_(NO_CACHED_DATA),
+      cached_parse_data_(NULL),
       ast_value_factory_(NULL),
       info_(info),
       has_pending_error_(false),
@@ -805,10 +854,10 @@ FunctionLiteral* Parser::ParseProgram() {
 
   // Initialize parser state.
   CompleteParserRecorder recorder;
-  if (cached_data_mode_ == PRODUCE_CACHED_DATA) {
+  if (cached_data_mode() == PRODUCE_CACHED_DATA) {
     log_ = &recorder;
-  } else if (cached_data_mode_ == CONSUME_CACHED_DATA) {
-    (*cached_data_)->Initialize();
+  } else if (cached_data_mode() == CONSUME_CACHED_DATA) {
+    cached_parse_data_->Initialize();
   }
 
   source = String::Flatten(source);
@@ -840,11 +889,8 @@ FunctionLiteral* Parser::ParseProgram() {
     }
     PrintF(" - took %0.3f ms]\n", ms);
   }
-  if (cached_data_mode_ == PRODUCE_CACHED_DATA) {
-    if (result != NULL) {
-      Vector<unsigned> store = recorder.ExtractData();
-      *cached_data_ = new ScriptData(store);
-    }
+  if (cached_data_mode() == PRODUCE_CACHED_DATA) {
+    if (result != NULL) *info_->cached_data() = recorder.GetScriptData();
     log_ = NULL;
   }
   return result;
@@ -3293,12 +3339,6 @@ DebuggerStatement* Parser::ParseDebuggerStatement(bool* ok) {
 }
 
 
-void Parser::ReportInvalidCachedData(const AstRawString* name, bool* ok) {
-  ParserTraits::ReportMessage("invalid_cached_data_function", name);
-  *ok = false;
-}
-
-
 bool CompileTimeValue::IsCompileTimeValue(Expression* expression) {
   if (expression->IsLiteral()) return true;
   MaterializedLiteral* lit = expression->AsMaterializedLiteral();
@@ -3615,37 +3655,27 @@ void Parser::SkipLazyFunctionBody(const AstRawString* function_name,
                                   int* expected_property_count,
                                   bool* ok) {
   int function_block_pos = position();
-  if (cached_data_mode_ == CONSUME_CACHED_DATA) {
+  if (cached_data_mode() == CONSUME_CACHED_DATA) {
     // If we have cached data, we use it to skip parsing the function body. The
     // data contains the information we need to construct the lazy function.
     FunctionEntry entry =
-        (*cached_data())->GetFunctionEntry(function_block_pos);
-    if (entry.is_valid()) {
-      if (entry.end_pos() <= function_block_pos) {
-        // End position greater than end of stream is safe, and hard to check.
-        ReportInvalidCachedData(function_name, ok);
-        if (!*ok) {
-          return;
-        }
-      }
-      scanner()->SeekForward(entry.end_pos() - 1);
-
-      scope_->set_end_position(entry.end_pos());
-      Expect(Token::RBRACE, ok);
-      if (!*ok) {
-        return;
-      }
-      isolate()->counters()->total_preparse_skipped()->Increment(
-          scope_->end_position() - function_block_pos);
-      *materialized_literal_count = entry.literal_count();
-      *expected_property_count = entry.property_count();
-      scope_->SetStrictMode(entry.strict_mode());
-    } else {
-      // This case happens when we have preparse data but it doesn't contain an
-      // entry for the function. Fail the compilation.
-      ReportInvalidCachedData(function_name, ok);
+        cached_parse_data_->GetFunctionEntry(function_block_pos);
+    // Invalid cached data if checks fail.

[vogelheim, 2014/07/09 17:23:36]

I can't really parse the comment.

[Yang, 2014/07/10 08:28:45]

Changed.

+    CHECK(entry.is_valid());
+    // End position greater than end of stream is safe, and hard to check.
+    CHECK(entry.end_pos() > function_block_pos);
+    scanner()->SeekForward(entry.end_pos() - 1);
+
+    scope_->set_end_position(entry.end_pos());
+    Expect(Token::RBRACE, ok);
+    if (!*ok) {
       return;
     }
+    isolate()->counters()->total_preparse_skipped()->Increment(
+        scope_->end_position() - function_block_pos);
+    *materialized_literal_count = entry.literal_count();
+    *expected_property_count = entry.property_count();
+    scope_->SetStrictMode(entry.strict_mode());
   } else {
     // With no cached data, we partially parse the function, without building an
     // AST. This gathers the data needed to build a lazy function.
@@ -3675,7 +3705,7 @@ void Parser::SkipLazyFunctionBody(const AstRawString* function_name,
     *materialized_literal_count = logger.literals();
     *expected_property_count = logger.properties();
     scope_->SetStrictMode(logger.strict_mode());
-    if (cached_data_mode_ == PRODUCE_CACHED_DATA) {
+    if (cached_data_mode() == PRODUCE_CACHED_DATA) {
       ASSERT(log_);
       // Position right after terminal '}'.
       int body_end = scanner()->location().end_pos;
@@ -4767,70 +4797,6 @@ RegExpTree* RegExpParser::ParseCharacterClass() {
 // ----------------------------------------------------------------------------
 // The Parser interface.
 
-ScriptData::~ScriptData() {
-  if (owns_store_) store_.Dispose();
-}
-
-
-int ScriptData::Length() {
-  return store_.length() * sizeof(unsigned);
-}
-
-
-const char* ScriptData::Data() {
-  return reinterpret_cast<const char*>(store_.start());
-}
-
-
-bool ScriptData::HasError() {
-  return has_error();
-}
-
-
-void ScriptData::Initialize() {
-  // Prepares state for use.
-  if (store_.length() >= PreparseDataConstants::kHeaderSize) {
-    function_index_ = PreparseDataConstants::kHeaderSize;
-    int symbol_data_offset = PreparseDataConstants::kHeaderSize
-        + store_[PreparseDataConstants::kFunctionsSizeOffset];
-    if (store_.length() > symbol_data_offset) {
-      symbol_data_ = reinterpret_cast<byte*>(&store_[symbol_data_offset]);
-    } else {
-      // Partial preparse causes no symbol information.
-      symbol_data_ = reinterpret_cast<byte*>(&store_[0] + store_.length());
-    }
-    symbol_data_end_ = reinterpret_cast<byte*>(&store_[0] + store_.length());
-  }
-}
-
-
-int ScriptData::ReadNumber(byte** source) {
-  // Reads a number from symbol_data_ in base 128. The most significant
-  // bit marks that there are more digits.
-  // If the first byte is 0x80 (kNumberTerminator), it would normally
-  // represent a leading zero. Since that is useless, and therefore won't
-  // appear as the first digit of any actual value, it is used to
-  // mark the end of the input stream.
-  byte* data = *source;
-  if (data >= symbol_data_end_) return -1;
-  byte input = *data;
-  if (input == PreparseDataConstants::kNumberTerminator) {
-    // End of stream marker.
-    return -1;
-  }
-  int result = input & 0x7f;
-  data++;
-  while ((input & 0x80u) != 0) {
-    if (data >= symbol_data_end_) return -1;
-    input = *data;
-    result = (result << 7) | (input & 0x7f);
-    data++;
-  }
-  *source = data;
-  return result;
-}
-
-
 bool RegExpParser::ParseRegExp(FlatStringReader* input,
                                bool multiline,
                                RegExpCompileData* result,
@@ -4877,7 +4843,7 @@ bool Parser::Parse() {
       result = ParseProgram();
     }
   } else {
-    SetCachedData(info()->cached_data(), info()->cached_data_mode());
+    SetCachedData();
     result = ParseProgram();
   }
   info()->SetFunction(result);