Revision: 21146
Author: ma...@chromium.org
Date: Mon May 5 14:55:13 2014 UTC
Log: 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=dcar...@chromium.org
Review URL: https://codereview.chromium.org/261273003
http://code.google.com/p/v8/source/detail?r=21146
Modified:
/branches/bleeding_edge/src/parser.cc
/branches/bleeding_edge/src/parser.h
/branches/bleeding_edge/src/preparse-data-format.h
/branches/bleeding_edge/src/preparse-data.cc
/branches/bleeding_edge/src/preparse-data.h
/branches/bleeding_edge/src/scanner.cc
/branches/bleeding_edge/src/scanner.h
/branches/bleeding_edge/test/cctest/test-parsing.cc
=======================================
--- /branches/bleeding_edge/src/parser.cc Wed Apr 30 10:51:01 2014 UTC
+++ /branches/bleeding_edge/src/parser.cc Mon May 5 14:55:13 2014 UTC
@@ -180,25 +180,6 @@
new(zone()) RegExpQuantifier(min, max, quantifier_type, atom),
zone());
LAST(ADD_TERM);
}
-
-
-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) {
@@ -280,10 +261,6 @@
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 @@
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 @@
info->zone(),
this),
isolate_(info->isolate()),
- symbol_cache_(0, info->zone()),
script_(info->script()),
scanner_(isolate_->unicode_cache()),
reusable_preparser_(NULL),
=======================================
--- /branches/bleeding_edge/src/parser.h Tue Apr 29 06:42:26 2014 UTC
+++ /branches/bleeding_edge/src/parser.h Mon May 5 14:55:13 2014 UTC
@@ -91,11 +91,6 @@
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 @@
} else {
ASSERT(data != NULL);
cached_data_ = data;
- symbol_cache_.Initialize(*data ? (*data)->symbol_count() : 0,
zone());
}
}
@@ -769,8 +763,6 @@
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 @@
bool* ok);
Isolate* isolate_;
- ZoneList<Handle<String> > symbol_cache_;
Handle<Script> script_;
Scanner scanner_;
=======================================
--- /branches/bleeding_edge/src/preparse-data-format.h Tue Apr 29 06:42:26
2014 UTC
+++ /branches/bleeding_edge/src/preparse-data-format.h Mon May 5 14:55:13
2014 UTC
@@ -14,15 +14,14 @@
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;
=======================================
--- /branches/bleeding_edge/src/preparse-data.cc Tue Apr 29 06:42:26 2014
UTC
+++ /branches/bleeding_edge/src/preparse-data.cc Mon May 5 14:55:13 2014
UTC
@@ -13,50 +13,18 @@
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
@@ -91,92 +59,20 @@
function_store_.Add(str[i]);
}
}
-
-
-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.
=======================================
--- /branches/bleeding_edge/src/preparse-data.h Tue Apr 29 06:42:26 2014 UTC
+++ /branches/bleeding_edge/src/preparse-data.h Mon May 5 14:55:13 2014 UTC
@@ -34,16 +34,6 @@
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 @@
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 @@
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 @@
#ifdef DEBUG
int prev_start_;
#endif
-
- Collector<byte> literal_chars_;
- Collector<byte> symbol_store_;
- Collector<Key> symbol_keys_;
- HashMap string_table_;
- int symbol_id_;
};
=======================================
--- /branches/bleeding_edge/src/scanner.cc Wed Apr 30 09:13:21 2014 UTC
+++ /branches/bleeding_edge/src/scanner.cc Mon May 5 14:55:13 2014 UTC
@@ -1136,15 +1136,6 @@
}
return finder->AddTwoByteSymbol(literal_two_byte_string(), 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) {
=======================================
--- /branches/bleeding_edge/src/scanner.h Tue Apr 29 06:42:26 2014 UTC
+++ /branches/bleeding_edge/src/scanner.h Mon May 5 14:55:13 2014 UTC
@@ -405,8 +405,6 @@
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.
=======================================
--- /branches/bleeding_edge/test/cctest/test-parsing.cc Thu Apr 17 13:27:02
2014 UTC
+++ /branches/bleeding_edge/test/cctest/test-parsing.cc Mon May 5 14:55:13
2014 UTC
@@ -275,56 +275,6 @@
CHECK(result->IsInt32());
CHECK_EQ(25, result->Int32Value());
}
-
-
-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) {
@@ -433,53 +383,6 @@
v8::String::Utf8Value utf8(result);
CHECK_EQ("foo", *utf8);
}
-}
-
-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());
- }
}
@@ -2072,28 +1975,19 @@
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 @@
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"
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.