From 98cd283399207202de567d772dfd65455f36e122 Mon Sep 17 00:00:00 2001
From: "marja@chromium.org"
 <marja@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Date: Mon, 5 May 2014 14:55:13 +0000
Subject: [PATCH] Remove symbol preparse data altogether.

Removing it seems to be a clear win on mobile: producing symbol data makes cold
parsing 20-30% slower, and having symbol data doesn't make warm parsing any
faster.

Notes:
- V8 used to produce symbol data, but because of a bug, it was never used until
recently. (See fix https://codereview.chromium.org/172753002 which takes the
symbol data into use again.)
- On desktop, warm parsing is faster if we have symbol data, and producing it
during cold parsing doesn't make parsing substantially slower. However, this
doesn't seem to be the case on mobile.
- The preparse data (cached data) will now contain only the positions of the
lazy functions.

BUG=
R=dcarney@chromium.org

Review URL: https://codereview.chromium.org/261273003

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@21146 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---
 src/parser.cc               |  36 ------------
 src/parser.h                |   9 ---
 src/preparse-data-format.h  |   7 +--
 src/preparse-data.cc        | 112 ++----------------------------------
 src/preparse-data.h         |  27 ---------
 src/scanner.cc              |   9 ---
 src/scanner.h               |   2 -
 test/cctest/test-parsing.cc | 136 ++++----------------------------------------
 8 files changed, 18 insertions(+), 320 deletions(-)

diff --git a/src/parser.cc b/src/parser.cc
index 4b4594b..0ff11f2 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -182,25 +182,6 @@ void RegExpBuilder::AddQuantifierToAtom(
 }
 
 
-Handle<String> Parser::LookupCachedSymbol(int symbol_id) {
-  // Make sure the cache is large enough to hold the symbol identifier.
-  if (symbol_cache_.length() <= symbol_id) {
-    // Increase length to index + 1.
-    symbol_cache_.AddBlock(Handle<String>::null(),
-                           symbol_id + 1 - symbol_cache_.length(), zone());
-  }
-  Handle<String> result = symbol_cache_.at(symbol_id);
-  if (result.is_null()) {
-    result = scanner()->AllocateInternalizedString(isolate_);
-    ASSERT(!result.is_null());
-    symbol_cache_.at(symbol_id) = result;
-    return result;
-  }
-  isolate()->counters()->total_preparse_symbols_skipped()->Increment();
-  return result;
-}
-
-
 ScriptData* ScriptData::New(const char* data, int length) {
   // The length is obviously invalid.
   if (length % sizeof(unsigned) != 0) {
@@ -280,10 +261,6 @@ bool ScriptData::SanityCheck() {
       static_cast<int>(store_[PreparseDataConstants::kFunctionsSizeOffset]);
   if (functions_size < 0) return false;
   if (functions_size % FunctionEntry::kSize != 0) return false;
-  // Check that the count of symbols is non-negative.
-  int symbol_count =
-      static_cast<int>(store_[PreparseDataConstants::kSymbolCountOffset]);
-  if (symbol_count < 0) return false;
   // Check that the total size has room for header and function entries.
   int minimum_size =
       PreparseDataConstants::kHeaderSize + functions_size;
@@ -707,18 +684,6 @@ void ParserTraits::ReportMessageAt(Scanner::Location source_location,
 
 
 Handle<String> ParserTraits::GetSymbol(Scanner* scanner) {
-  if (parser_->cached_data_mode() == CONSUME_CACHED_DATA) {
-    int symbol_id = (*parser_->cached_data())->GetSymbolIdentifier();
-    // If there is no symbol data, -1 will be returned.
-    if (symbol_id >= 0 &&
-        symbol_id < (*parser_->cached_data())->symbol_count()) {
-      return parser_->LookupCachedSymbol(symbol_id);
-    }
-  } else if (parser_->cached_data_mode() == PRODUCE_CACHED_DATA) {
-    // Parser is never used inside lazy functions (it falls back to PreParser
-    // instead), so we can produce the symbol data unconditionally.
-    parser_->scanner()->LogSymbol(parser_->log_, parser_->position());
-  }
   Handle<String> result =
       parser_->scanner()->AllocateInternalizedString(parser_->isolate());
   ASSERT(!result.is_null());
@@ -819,7 +784,6 @@ Parser::Parser(CompilationInfo* info)
                                info->zone(),
                                this),
       isolate_(info->isolate()),
-      symbol_cache_(0, info->zone()),
       script_(info->script()),
       scanner_(isolate_->unicode_cache()),
       reusable_preparser_(NULL),
diff --git a/src/parser.h b/src/parser.h
index 8bc8a48..71bbfd1 100644
--- a/src/parser.h
+++ b/src/parser.h
@@ -91,11 +91,6 @@ class ScriptData {
   const char* BuildMessage() const;
   Vector<const char*> BuildArgs() const;
 
-  int symbol_count() {
-    return (store_.length() > PreparseDataConstants::kHeaderSize)
-        ? store_[PreparseDataConstants::kSymbolCountOffset]
-        : 0;
-  }
   int function_count() {
     int functions_size =
         static_cast<int>(store_[PreparseDataConstants::kFunctionsSizeOffset]);
@@ -658,7 +653,6 @@ class Parser : public ParserBase<ParserTraits> {
     } else {
       ASSERT(data != NULL);
       cached_data_ = data;
-      symbol_cache_.Initialize(*data ? (*data)->symbol_count() : 0, zone());
     }
   }
 
@@ -769,8 +763,6 @@ class Parser : public ParserBase<ParserTraits> {
 
   Scope* NewScope(Scope* parent, ScopeType type);
 
-  Handle<String> LookupCachedSymbol(int symbol_id);
-
   // Skip over a lazy function, either using cached data if we have it, or
   // by parsing the function with PreParser. Consumes the ending }.
   void SkipLazyFunctionBody(Handle<String> function_name,
@@ -790,7 +782,6 @@ class Parser : public ParserBase<ParserTraits> {
                                                bool* ok);
 
   Isolate* isolate_;
-  ZoneList<Handle<String> > symbol_cache_;
 
   Handle<Script> script_;
   Scanner scanner_;
diff --git a/src/preparse-data-format.h b/src/preparse-data-format.h
index 2167936..4d1ad7a 100644
--- a/src/preparse-data-format.h
+++ b/src/preparse-data-format.h
@@ -14,15 +14,14 @@ struct PreparseDataConstants {
  public:
   // Layout and constants of the preparse data exchange format.
   static const unsigned kMagicNumber = 0xBadDead;
-  static const unsigned kCurrentVersion = 8;
+  static const unsigned kCurrentVersion = 9;
 
   static const int kMagicOffset = 0;
   static const int kVersionOffset = 1;
   static const int kHasErrorOffset = 2;
   static const int kFunctionsSizeOffset = 3;
-  static const int kSymbolCountOffset = 4;
-  static const int kSizeOffset = 5;
-  static const int kHeaderSize = 6;
+  static const int kSizeOffset = 4;
+  static const int kHeaderSize = 5;
 
   // If encoding a message, the following positions are fixed.
   static const int kMessageStartPos = 0;
diff --git a/src/preparse-data.cc b/src/preparse-data.cc
index a54eb82..5ddf721 100644
--- a/src/preparse-data.cc
+++ b/src/preparse-data.cc
@@ -15,48 +15,16 @@ namespace v8 {
 namespace internal {
 
 
-template <typename Char>
-static int vector_hash(Vector<const Char> string) {
-  int hash = 0;
-  for (int i = 0; i < string.length(); i++) {
-    int c = static_cast<int>(string[i]);
-    hash += c;
-    hash += (hash << 10);
-    hash ^= (hash >> 6);
-  }
-  return hash;
-}
-
-
-static bool vector_compare(void* a, void* b) {
-  CompleteParserRecorder::Key* string1 =
-      reinterpret_cast<CompleteParserRecorder::Key*>(a);
-  CompleteParserRecorder::Key* string2 =
-      reinterpret_cast<CompleteParserRecorder::Key*>(b);
-  if (string1->is_one_byte != string2->is_one_byte) return false;
-  int length = string1->literal_bytes.length();
-  if (string2->literal_bytes.length() != length) return false;
-  return memcmp(string1->literal_bytes.start(),
-                string2->literal_bytes.start(), length) == 0;
-}
-
-
 CompleteParserRecorder::CompleteParserRecorder()
-    : function_store_(0),
-      literal_chars_(0),
-      symbol_store_(0),
-      symbol_keys_(0),
-      string_table_(vector_compare),
-      symbol_id_(0) {
+    : function_store_(0) {
   preamble_[PreparseDataConstants::kMagicOffset] =
       PreparseDataConstants::kMagicNumber;
   preamble_[PreparseDataConstants::kVersionOffset] =
       PreparseDataConstants::kCurrentVersion;
   preamble_[PreparseDataConstants::kHasErrorOffset] = false;
   preamble_[PreparseDataConstants::kFunctionsSizeOffset] = 0;
-  preamble_[PreparseDataConstants::kSymbolCountOffset] = 0;
   preamble_[PreparseDataConstants::kSizeOffset] = 0;
-  ASSERT_EQ(6, PreparseDataConstants::kHeaderSize);
+  ASSERT_EQ(5, PreparseDataConstants::kHeaderSize);
 #ifdef DEBUG
   prev_start_ = -1;
 #endif
@@ -93,90 +61,18 @@ void CompleteParserRecorder::WriteString(Vector<const char> str) {
 }
 
 
-void CompleteParserRecorder::LogOneByteSymbol(int start,
-                                              Vector<const uint8_t> literal) {
-  int hash = vector_hash(literal);
-  LogSymbol(start, hash, true, literal);
-}
-
-
-void CompleteParserRecorder::LogTwoByteSymbol(int start,
-                                              Vector<const uint16_t> literal) {
-  int hash = vector_hash(literal);
-  LogSymbol(start, hash, false, Vector<const byte>::cast(literal));
-}
-
-
-void CompleteParserRecorder::LogSymbol(int start,
-                                       int hash,
-                                       bool is_one_byte,
-                                       Vector<const byte> literal_bytes) {
-  Key key = { is_one_byte, literal_bytes };
-  HashMap::Entry* entry = string_table_.Lookup(&key, hash, true);
-  int id = static_cast<int>(reinterpret_cast<intptr_t>(entry->value));
-  if (id == 0) {
-    // Copy literal contents for later comparison.
-    key.literal_bytes =
-        Vector<const byte>::cast(literal_chars_.AddBlock(literal_bytes));
-    // Put (symbol_id_ + 1) into entry and increment it.
-    id = ++symbol_id_;
-    entry->value = reinterpret_cast<void*>(id);
-    Vector<Key> symbol = symbol_keys_.AddBlock(1, key);
-    entry->key = &symbol[0];
-  }
-  WriteNumber(id - 1);
-}
-
-
 Vector<unsigned> CompleteParserRecorder::ExtractData() {
   int function_size = function_store_.size();
-  // Add terminator to symbols, then pad to unsigned size.
-  int symbol_size = symbol_store_.size();
-  int padding = sizeof(unsigned) - (symbol_size % sizeof(unsigned));
-  symbol_store_.AddBlock(padding, PreparseDataConstants::kNumberTerminator);
-  symbol_size += padding;
-  int total_size = PreparseDataConstants::kHeaderSize + function_size
-      + (symbol_size / sizeof(unsigned));
+  int total_size = PreparseDataConstants::kHeaderSize + function_size;
   Vector<unsigned> data = Vector<unsigned>::New(total_size);
   preamble_[PreparseDataConstants::kFunctionsSizeOffset] = function_size;
-  preamble_[PreparseDataConstants::kSymbolCountOffset] = symbol_id_;
   OS::MemCopy(data.start(), preamble_, sizeof(preamble_));
-  int symbol_start = PreparseDataConstants::kHeaderSize + function_size;
   if (function_size > 0) {
     function_store_.WriteTo(data.SubVector(PreparseDataConstants::kHeaderSize,
-                                           symbol_start));
-  }
-  if (!has_error()) {
-    symbol_store_.WriteTo(
-        Vector<byte>::cast(data.SubVector(symbol_start, total_size)));
+                                           total_size));
   }
   return data;
 }
 
 
-void CompleteParserRecorder::WriteNumber(int number) {
-  // Split the number into chunks of 7 bits. Write them one after another (the
-  // most significant first). Use the MSB of each byte for signalling that the
-  // number continues. See ScriptDataImpl::ReadNumber for the reading side.
-  ASSERT(number >= 0);
-
-  int mask = (1 << 28) - 1;
-  int i = 28;
-  // 26 million symbols ought to be enough for anybody.
-  ASSERT(number <= mask);
-  while (number < mask) {
-    mask >>= 7;
-    i -= 7;
-  }
-  while (i > 0) {
-    symbol_store_.Add(static_cast<byte>(number >> i) | 0x80u);
-    number &= mask;
-    mask >>= 7;
-    i -= 7;
-  }
-  ASSERT(number < (1 << 7));
-  symbol_store_.Add(static_cast<byte>(number));
-}
-
-
 } }  // namespace v8::internal.
diff --git a/src/preparse-data.h b/src/preparse-data.h
index b8cf23b..051a909 100644
--- a/src/preparse-data.h
+++ b/src/preparse-data.h
@@ -34,16 +34,6 @@ class ParserRecorder {
                           const char* message,
                           const char* argument_opt,
                           bool is_reference_error) = 0;
-
-  // The following functions are only callable on CompleteParserRecorder
-  // and are guarded by calls to ShouldLogSymbols.
-  virtual void LogOneByteSymbol(int start, Vector<const uint8_t> literal) {
-    UNREACHABLE();
-  }
-  virtual void LogTwoByteSymbol(int start, Vector<const uint16_t> literal) {
-    UNREACHABLE();
-  }
-
  private:
   DISALLOW_COPY_AND_ASSIGN(ParserRecorder);
 };
@@ -158,9 +148,6 @@ class CompleteParserRecorder : public ParserRecorder {
                           const char* message,
                           const char* argument_opt,
                           bool is_reference_error_);
-
-  virtual void LogOneByteSymbol(int start, Vector<const uint8_t> literal);
-  virtual void LogTwoByteSymbol(int start, Vector<const uint16_t> literal);
   Vector<unsigned> ExtractData();
 
  private:
@@ -170,14 +157,6 @@ class CompleteParserRecorder : public ParserRecorder {
 
   void WriteString(Vector<const char> str);
 
-  // For testing. Defined in test-parsing.cc.
-  friend struct CompleteParserRecorderFriend;
-
-  void LogSymbol(int start,
-                 int hash,
-                 bool is_one_byte,
-                 Vector<const byte> literal);
-
   // Write a non-negative number to the symbol store.
   void WriteNumber(int number);
 
@@ -187,12 +166,6 @@ class CompleteParserRecorder : public ParserRecorder {
 #ifdef DEBUG
   int prev_start_;
 #endif
-
-  Collector<byte> literal_chars_;
-  Collector<byte> symbol_store_;
-  Collector<Key> symbol_keys_;
-  HashMap string_table_;
-  int symbol_id_;
 };
 
 
diff --git a/src/scanner.cc b/src/scanner.cc
index 4ad0f41..2e039ca 100644
--- a/src/scanner.cc
+++ b/src/scanner.cc
@@ -1138,15 +1138,6 @@ int Scanner::FindSymbol(DuplicateFinder* finder, int value) {
 }
 
 
-void Scanner::LogSymbol(ParserRecorder* log, int position) {
-  if (is_literal_one_byte()) {
-    log->LogOneByteSymbol(position, literal_one_byte_string());
-  } else {
-    log->LogTwoByteSymbol(position, literal_two_byte_string());
-  }
-}
-
-
 int DuplicateFinder::AddOneByteSymbol(Vector<const uint8_t> key, int value) {
   return AddSymbol(key, true, value);
 }
diff --git a/src/scanner.h b/src/scanner.h
index 052bf73..037da5b 100644
--- a/src/scanner.h
+++ b/src/scanner.h
@@ -405,8 +405,6 @@ class Scanner {
   int FindNumber(DuplicateFinder* finder, int value);
   int FindSymbol(DuplicateFinder* finder, int value);
 
-  void LogSymbol(ParserRecorder* log, int position);
-
   UnicodeCache* unicode_cache() { return unicode_cache_; }
 
   // Returns the location of the last seen octal literal.
diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc
index e0c21f4..58734d0 100644
--- a/test/cctest/test-parsing.cc
+++ b/test/cctest/test-parsing.cc
@@ -277,56 +277,6 @@ TEST(PreparseFunctionDataIsUsed) {
 }
 
 
-TEST(PreparseSymbolDataIsUsed) {
-  // This tests that we actually do use the symbol data generated by the
-  // preparser.
-
-  // Only do one compilation pass in this test (otherwise we will parse the
-  // source code again without preparse data and it will fail).
-  i::FLAG_crankshaft = false;
-
-  // Make preparsing work for short scripts.
-  i::FLAG_min_preparse_length = 0;
-
-  v8::Isolate* isolate = CcTest::isolate();
-  v8::HandleScope handles(isolate);
-  v8::Local<v8::Context> context = v8::Context::New(isolate);
-  v8::Context::Scope context_scope(context);
-  int marker;
-  CcTest::i_isolate()->stack_guard()->SetStackLimit(
-      reinterpret_cast<uintptr_t>(&marker) - 128 * 1024);
-
-  // Note that the ( before function makes the function not lazily compiled.
-  const char* good_code =
-      "(function weird() { var foo = 26; return foo; })()";
-
-  // Insert an undefined identifier. If the preparser data is used, the symbol
-  // stream is used instead, and this identifier resolves to "foo".
-  const char* bad_code =
-      "(function weird() { var foo = 26; return wut; })()";
-
-  v8::ScriptCompiler::Source good_source(v8_str(good_code));
-  v8::ScriptCompiler::Compile(isolate, &good_source,
-                              v8::ScriptCompiler::kProduceDataToCache);
-
-  const v8::ScriptCompiler::CachedData* cached_data =
-      good_source.GetCachedData();
-  CHECK(cached_data->data != NULL);
-  CHECK_GT(cached_data->length, 0);
-
-  // Now compile the erroneous code with the good preparse data. If the preparse
-  // data is used, we will see a second occurrence of "foo" instead of the
-  // unknown "wut".
-  v8::ScriptCompiler::Source bad_source(
-      v8_str(bad_code), new v8::ScriptCompiler::CachedData(
-                            cached_data->data, cached_data->length));
-  v8::Local<v8::Value> result =
-      v8::ScriptCompiler::Compile(isolate, &bad_source)->Run();
-  CHECK(result->IsInt32());
-  CHECK_EQ(26, result->Int32Value());
-}
-
-
 TEST(StandAlonePreParser) {
   v8::V8::Initialize();
 
@@ -435,53 +385,6 @@ TEST(PreparsingObjectLiterals) {
   }
 }
 
-namespace v8 {
-namespace internal {
-
-struct CompleteParserRecorderFriend {
-  static void FakeWritingSymbolIdInPreParseData(CompleteParserRecorder* log,
-                                                int number) {
-    log->WriteNumber(number);
-    if (log->symbol_id_ < number + 1) {
-      log->symbol_id_ = number + 1;
-    }
-  }
-};
-
-}
-}
-
-
-TEST(StoringNumbersInPreParseData) {
-  // Symbol IDs are split into chunks of 7 bits for storing. This is a
-  // regression test for a bug where a symbol id was incorrectly stored if some
-  // of the chunks in the middle were all zeros.
-  typedef i::CompleteParserRecorderFriend F;
-  i::CompleteParserRecorder log;
-  for (int i = 0; i < 18; ++i) {
-    F::FakeWritingSymbolIdInPreParseData(&log, 1 << i);
-  }
-  for (int i = 1; i < 18; ++i) {
-    F::FakeWritingSymbolIdInPreParseData(&log, (1 << i) + 1);
-  }
-  for (int i = 6; i < 18; ++i) {
-    F::FakeWritingSymbolIdInPreParseData(&log, (3 << i) + (5 << (i - 6)));
-  }
-  i::Vector<unsigned> store = log.ExtractData();
-  i::ScriptData script_data(store);
-  script_data.Initialize();
-  // Check that we get the same symbols back.
-  for (int i = 0; i < 18; ++i) {
-    CHECK_EQ(1 << i, script_data.GetSymbolIdentifier());
-  }
-  for (int i = 1; i < 18; ++i) {
-    CHECK_EQ((1 << i) + 1, script_data.GetSymbolIdentifier());
-  }
-  for (int i = 6; i < 18; ++i) {
-    CHECK_EQ((3 << i) + (5 << (i - 6)), script_data.GetSymbolIdentifier());
-  }
-}
-
 
 TEST(RegressChromium62639) {
   v8::V8::Initialize();
@@ -2072,28 +1975,19 @@ TEST(DontRegressPreParserDataSizes) {
 
   struct TestCase {
     const char* program;
-    int symbols;
     int functions;
   } test_cases[] = {
-    // Labels and variables are recorded as symbols.
-    {"{label: 42}", 1, 0}, {"{label: 42; label2: 43}", 2, 0},
-    {"var x = 42;", 1, 0}, {"var x = 42, y = 43;", 2, 0},
-    {"var x = {y: 1};", 2, 0},
-    {"var x = {}; x.y = 1", 2, 0},
-    // "get" is recorded as a symbol too.
-    {"var x = {get foo(){} };", 3, 1},
-    // When keywords are used as identifiers, they're logged as symbols, too:
-    {"var x = {if: 1};", 2, 0},
-    {"var x = {}; x.if = 1", 2, 0},
-    {"var x = {get if(){} };", 3, 1},
-    // Functions
-    {"function foo() {}", 1, 1}, {"function foo() {} function bar() {}", 2, 2},
-    // Labels, variables and functions insize lazy functions are not recorded.
-    {"function lazy() { var a, b, c; }", 1, 1},
-    {"function lazy() { a: 1; b: 2; c: 3; }", 1, 1},
-    {"function lazy() { function a() {} function b() {} function c() {} }", 1,
-     1},
-    {NULL, 0, 0}
+    // No functions.
+    {"var x = 42;", 0},
+    // Functions.
+    {"function foo() {}", 1}, {"function foo() {} function bar() {}", 2},
+    // Getter / setter functions are recorded as functions if they're on the top
+    // level.
+    {"var x = {get foo(){} };", 1},
+    // Functions insize lazy functions are not recorded.
+    {"function lazy() { function a() {} function b() {} function c() {} }", 1},
+    {"function lazy() { var x = {get foo(){} } }", 1},
+    {NULL, 0}
   };
 
   for (int i = 0; test_cases[i].program; i++) {
@@ -2109,14 +2003,6 @@ TEST(DontRegressPreParserDataSizes) {
     CHECK(data);
     CHECK(!data->HasError());
 
-    if (data->symbol_count() != test_cases[i].symbols) {
-      i::OS::Print(
-          "Expected preparse data for program:\n"
-          "\t%s\n"
-          "to contain %d symbols, however, received %d symbols.\n",
-          program, test_cases[i].symbols, data->symbol_count());
-      CHECK(false);
-    }
     if (data->function_count() != test_cases[i].functions) {
       i::OS::Print(
           "Expected preparse data for program:\n"
-- 
2.7.4