From 6d163d9fd0fdef82558944f505f6773824bb2bee Mon Sep 17 00:00:00 2001
From: "sgjesse@chromium.org"
 <sgjesse@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Date: Fri, 27 Nov 2009 14:10:48 +0000
Subject: [PATCH] Remove usage of JSArray in Script object

Storing a JSArray in the Script object could cause an indirect reference from the compilation cache to a global object to be created. Now the line ends are only stored as a FixedArrya and when that is needed in JavaScript a JSArray copy is created. Changed some of the JavaScript code to cache the line ends in a local variable for better performance.

BUG=http://code.google.com/p/v8/issues/detail?id=528
TEST=test/test-api/Bug528
Review URL: http://codereview.chromium.org/434117

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@3374 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---
 include/v8.h              |  4 ++--
 src/accessors.cc          | 13 +++++--------
 src/api.cc                |  4 ++--
 src/factory.cc            |  3 +--
 src/handles.cc            | 12 ++++++------
 src/messages.js           | 24 ++++++++++++++----------
 src/objects-debug.cc      |  9 +++------
 src/objects-inl.h         |  3 +--
 src/objects.h             | 17 +++--------------
 test/cctest/test-api.cc   | 22 ++--------------------
 test/cctest/test-debug.cc |  8 ++++----
 11 files changed, 43 insertions(+), 76 deletions(-)

diff --git a/include/v8.h b/include/v8.h
index f679b0e..a8ee8d4 100644
--- a/include/v8.h
+++ b/include/v8.h
@@ -598,7 +598,7 @@ class V8EXPORT Script {
    * with the debugger as this data object is only available through the
    * debugger API.
    */
-  void SetData(Handle<Value> data);
+  void SetData(Handle<String> data);
 };
 
 
@@ -2634,7 +2634,7 @@ class V8EXPORT Context {
    * with the debugger to provide additional information on the context through
    * the debugger API.
    */
-  void SetData(Handle<Value> data);
+  void SetData(Handle<String> data);
   Local<Value> GetData();
 
   /**
diff --git a/src/accessors.cc b/src/accessors.cc
index 734c364..09a3886 100644
--- a/src/accessors.cc
+++ b/src/accessors.cc
@@ -315,14 +315,11 @@ Object* Accessors::ScriptGetLineEnds(Object* object, void*) {
   HandleScope scope;
   Handle<Script> script(Script::cast(JSValue::cast(object)->value()));
   InitScriptLineEnds(script);
-  if (script->line_ends_js_array()->IsUndefined()) {
-    Handle<FixedArray> line_ends_fixed_array(
-        FixedArray::cast(script->line_ends_fixed_array()));
-    Handle<FixedArray> copy = Factory::CopyFixedArray(line_ends_fixed_array);
-    Handle<JSArray> js_array = Factory::NewJSArrayWithElements(copy);
-    script->set_line_ends_js_array(*js_array);
-  }
-  return script->line_ends_js_array();
+  ASSERT(script->line_ends()->IsFixedArray());
+  Handle<FixedArray> line_ends(FixedArray::cast(script->line_ends()));
+  Handle<FixedArray> copy = Factory::CopyFixedArray(line_ends);
+  Handle<JSArray> js_array = Factory::NewJSArrayWithElements(copy);
+  return *js_array;
 }
 
 
diff --git a/src/api.cc b/src/api.cc
index 220788b..3a2f8ab 100644
--- a/src/api.cc
+++ b/src/api.cc
@@ -451,7 +451,7 @@ void Context::Exit() {
 }
 
 
-void Context::SetData(v8::Handle<Value> data) {
+void Context::SetData(v8::Handle<String> data) {
   if (IsDeadCheck("v8::Context::SetData()")) return;
   ENTER_V8;
   {
@@ -1175,7 +1175,7 @@ Local<Value> Script::Id() {
 }
 
 
-void Script::SetData(v8::Handle<Value> data) {
+void Script::SetData(v8::Handle<String> data) {
   ON_BAILOUT("v8::Script::SetData()", return);
   LOG_API("Script::SetData");
   {
diff --git a/src/factory.cc b/src/factory.cc
index 3d9cd7a..c63e005 100644
--- a/src/factory.cc
+++ b/src/factory.cc
@@ -188,8 +188,7 @@ Handle<Script> Factory::NewScript(Handle<String> source) {
   script->set_type(Smi::FromInt(Script::TYPE_NORMAL));
   script->set_compilation_type(Smi::FromInt(Script::COMPILATION_TYPE_HOST));
   script->set_wrapper(*wrapper);
-  script->set_line_ends_fixed_array(Heap::undefined_value());
-  script->set_line_ends_js_array(Heap::undefined_value());
+  script->set_line_ends(Heap::undefined_value());
   script->set_eval_from_function(Heap::undefined_value());
   script->set_eval_from_instructions_offset(Smi::FromInt(0));
 
diff --git a/src/handles.cc b/src/handles.cc
index b42ad24..53fa01b 100644
--- a/src/handles.cc
+++ b/src/handles.cc
@@ -429,12 +429,12 @@ Handle<JSValue> GetScriptWrapper(Handle<Script> script) {
 // Init line_ends array with code positions of line ends inside script
 // source.
 void InitScriptLineEnds(Handle<Script> script) {
-  if (!script->line_ends_fixed_array()->IsUndefined()) return;
+  if (!script->line_ends()->IsUndefined()) return;
 
   if (!script->source()->IsString()) {
     ASSERT(script->source()->IsUndefined());
-    script->set_line_ends_fixed_array(*(Factory::NewFixedArray(0)));
-    ASSERT(script->line_ends_fixed_array()->IsFixedArray());
+    script->set_line_ends(*(Factory::NewFixedArray(0)));
+    ASSERT(script->line_ends()->IsFixedArray());
     return;
   }
 
@@ -467,8 +467,8 @@ void InitScriptLineEnds(Handle<Script> script) {
   }
   ASSERT(array_index == line_count);
 
-  script->set_line_ends_fixed_array(*array);
-  ASSERT(script->line_ends_fixed_array()->IsFixedArray());
+  script->set_line_ends(*array);
+  ASSERT(script->line_ends()->IsFixedArray());
 }
 
 
@@ -477,7 +477,7 @@ int GetScriptLineNumber(Handle<Script> script, int code_pos) {
   InitScriptLineEnds(script);
   AssertNoAllocation no_allocation;
   FixedArray* line_ends_array =
-      FixedArray::cast(script->line_ends_fixed_array());
+      FixedArray::cast(script->line_ends());
   const int line_ends_len = line_ends_array->length();
 
   int line = -1;
diff --git a/src/messages.js b/src/messages.js
index 2720792..b0004b1 100644
--- a/src/messages.js
+++ b/src/messages.js
@@ -238,14 +238,15 @@ function MakeError(type, args) {
 Script.prototype.lineFromPosition = function(position) {
   var lower = 0;
   var upper = this.lineCount() - 1;
+  var line_ends = this.line_ends;
 
   // We'll never find invalid positions so bail right away.
-  if (position > this.line_ends[upper]) {
+  if (position > line_ends[upper]) {
     return -1;
   }
 
   // This means we don't have to safe-guard indexing line_ends[i - 1].
-  if (position <= this.line_ends[0]) {
+  if (position <= line_ends[0]) {
     return 0;
   }
 
@@ -253,9 +254,9 @@ Script.prototype.lineFromPosition = function(position) {
   while (upper >= 1) {
     var i = (lower + upper) >> 1;
 
-    if (position > this.line_ends[i]) {
+    if (position > line_ends[i]) {
       lower = i + 1;
-    } else if (position <= this.line_ends[i - 1]) {
+    } else if (position <= line_ends[i - 1]) {
       upper = i - 1;
     } else {
       return i;
@@ -278,8 +279,9 @@ Script.prototype.locationFromPosition = function (position,
   if (line == -1) return null;
 
   // Determine start, end and column.
-  var start = line == 0 ? 0 : this.line_ends[line - 1] + 1;
-  var end = this.line_ends[line];
+  var line_ends = this.line_ends;
+  var start = line == 0 ? 0 : line_ends[line - 1] + 1;
+  var end = line_ends[line];
   if (end > 0 && StringCharAt.call(this.source, end - 1) == '\r') end--;
   var column = position - start;
 
@@ -368,8 +370,9 @@ Script.prototype.sourceSlice = function (opt_from_line, opt_to_line) {
     return null;
   }
 
-  var from_position = from_line == 0 ? 0 : this.line_ends[from_line - 1] + 1;
-  var to_position = to_line == 0 ? 0 : this.line_ends[to_line - 1] + 1;
+  var line_ends = this.line_ends;
+  var from_position = from_line == 0 ? 0 : line_ends[from_line - 1] + 1;
+  var to_position = to_line == 0 ? 0 : line_ends[to_line - 1] + 1;
 
   // Return a source slice with line numbers re-adjusted to the resource.
   return new SourceSlice(this, from_line + this.line_offset, to_line + this.line_offset,
@@ -391,8 +394,9 @@ Script.prototype.sourceLine = function (opt_line) {
   }
 
   // Return the source line.
-  var start = line == 0 ? 0 : this.line_ends[line - 1] + 1;
-  var end = this.line_ends[line];
+  var line_ends = this.line_ends;
+  var start = line == 0 ? 0 : line_ends[line - 1] + 1;
+  var end = line_ends[line];
   return StringSubstring.call(this.source, start, end);
 }
 
diff --git a/src/objects-debug.cc b/src/objects-debug.cc
index 8413124..c269d11 100644
--- a/src/objects-debug.cc
+++ b/src/objects-debug.cc
@@ -1116,8 +1116,7 @@ void Script::ScriptVerify() {
   VerifyPointer(data());
   VerifyPointer(wrapper());
   type()->SmiVerify();
-  VerifyPointer(line_ends_fixed_array());
-  VerifyPointer(line_ends_js_array());
+  VerifyPointer(line_ends());
   VerifyPointer(id());
 }
 
@@ -1144,10 +1143,8 @@ void Script::ScriptPrint() {
   wrapper()->ShortPrint();
   PrintF("\n - compilation type: ");
   compilation_type()->ShortPrint();
-  PrintF("\n - line ends fixed array: ");
-  line_ends_fixed_array()->ShortPrint();
-  PrintF("\n - line ends js array: ");
-  line_ends_js_array()->ShortPrint();
+  PrintF("\n - line ends: ");
+  line_ends()->ShortPrint();
   PrintF("\n - eval from function: ");
   eval_from_function()->ShortPrint();
   PrintF("\n - eval from instructions offset: ");
diff --git a/src/objects-inl.h b/src/objects-inl.h
index cf614d2..71e16ec 100644
--- a/src/objects-inl.h
+++ b/src/objects-inl.h
@@ -2324,8 +2324,7 @@ ACCESSORS(Script, context_data, Object, kContextOffset)
 ACCESSORS(Script, wrapper, Proxy, kWrapperOffset)
 ACCESSORS(Script, type, Smi, kTypeOffset)
 ACCESSORS(Script, compilation_type, Smi, kCompilationTypeOffset)
-ACCESSORS(Script, line_ends_fixed_array, Object, kLineEndsFixedArrayOffset)
-ACCESSORS(Script, line_ends_js_array, Object, kLineEndsJSArrayOffset)
+ACCESSORS(Script, line_ends, Object, kLineEndsOffset)
 ACCESSORS(Script, eval_from_function, Object, kEvalFromFunctionOffset)
 ACCESSORS(Script, eval_from_instructions_offset, Smi,
           kEvalFrominstructionsOffsetOffset)
diff --git a/src/objects.h b/src/objects.h
index b96ec4f..e6c44f3 100644
--- a/src/objects.h
+++ b/src/objects.h
@@ -3003,10 +3003,7 @@ class Script: public Struct {
   DECL_ACCESSORS(compilation_type, Smi)
 
   // [line_ends]: FixedArray of line ends positions.
-  DECL_ACCESSORS(line_ends_fixed_array, Object)
-
-  // [line_ends]: JSArray of line ends positions.
-  DECL_ACCESSORS(line_ends_js_array, Object)
+  DECL_ACCESSORS(line_ends, Object)
 
   // [eval_from_function]: for eval scripts the funcion from which eval was
   // called.
@@ -3036,16 +3033,8 @@ class Script: public Struct {
   static const int kWrapperOffset = kContextOffset + kPointerSize;
   static const int kTypeOffset = kWrapperOffset + kPointerSize;
   static const int kCompilationTypeOffset = kTypeOffset + kPointerSize;
-  // We have the line ends array both in FixedArray form and in JSArray form.
-  // The FixedArray form is useful when we don't have a context and so can't
-  // create a JSArray.  The JSArray form is useful when we want to see the
-  // array from JS code (e.g. debug-delay.js) which cannot handle unboxed
-  // FixedArray objects.
-  static const int kLineEndsFixedArrayOffset =
-      kCompilationTypeOffset + kPointerSize;
-  static const int kLineEndsJSArrayOffset =
-      kLineEndsFixedArrayOffset + kPointerSize;
-  static const int kIdOffset = kLineEndsJSArrayOffset + kPointerSize;
+  static const int kLineEndsOffset = kCompilationTypeOffset + kPointerSize;
+  static const int kIdOffset = kLineEndsOffset + kPointerSize;
   static const int kEvalFromFunctionOffset = kIdOffset + kPointerSize;
   static const int kEvalFrominstructionsOffsetOffset =
       kEvalFromFunctionOffset + kPointerSize;
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index c199cdb..d88c5e7 100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -8527,7 +8527,7 @@ TEST(Bug528) {
     v8::HandleScope scope;
 
     context->Enter();
-    Local<v8::Object> obj = v8::Object::New();
+    Local<v8::String> obj = v8::String::New("");
     context->SetData(obj);
     CompileRun("1");
     context->Exit();
@@ -8538,13 +8538,7 @@ TEST(Bug528) {
     if (GetGlobalObjectsCount() == 0) break;
   }
   CHECK_EQ(0, GetGlobalObjectsCount());
-
-  // Compilation cache size is different for Android.
-#if defined(ANDROID)
-  CHECK_EQ(1, gc_count);
-#else
-  CHECK_EQ(5, gc_count);
-#endif
+  CHECK_EQ(2, gc_count);
 
   // Eval in a function creates reference from the compilation cache to the
   // global object.
@@ -8562,13 +8556,7 @@ TEST(Bug528) {
     if (GetGlobalObjectsCount() == 0) break;
   }
   CHECK_EQ(0, GetGlobalObjectsCount());
-
-  // Compilation cache size is different for Android.
-#if defined(ANDROID)
-  CHECK_EQ(1, gc_count);
-#else
   CHECK_EQ(2, gc_count);
-#endif
 
   // Looking up the line number for an exception creates reference from the
   // compilation cache to the global object.
@@ -8591,11 +8579,5 @@ TEST(Bug528) {
     if (GetGlobalObjectsCount() == 0) break;
   }
   CHECK_EQ(0, GetGlobalObjectsCount());
-
-  // Compilation cache size is different for Android.
-#if defined(ANDROID)
   CHECK_EQ(2, gc_count);
-#else
-  CHECK_EQ(5, gc_count);
-#endif
 }
diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc
index d938174..5b72193 100644
--- a/test/cctest/test-debug.cc
+++ b/test/cctest/test-debug.cc
@@ -5016,7 +5016,7 @@ TEST(ScriptNameAndData) {
   v8::ScriptOrigin origin2 = v8::ScriptOrigin(v8::String::New("new name"));
   v8::Handle<v8::Script> script2 = v8::Script::Compile(script, &origin2);
   script2->Run();
-  script2->SetData(data_obj);
+  script2->SetData(data_obj->ToString());
   f = v8::Local<v8::Function>::Cast(env->Global()->Get(v8::String::New("f")));
   f->Call(env->Global(), 0, NULL);
   CHECK_EQ(3, break_point_hit_count);
@@ -5069,8 +5069,8 @@ TEST(ContextData) {
   CHECK(context_2->GetData()->IsUndefined());
 
   // Set and check different data values.
-  v8::Handle<v8::Value> data_1 = v8::Number::New(1);
-  v8::Handle<v8::Value> data_2 = v8::String::New("2");
+  v8::Handle<v8::String> data_1 = v8::String::New("1");
+  v8::Handle<v8::String> data_2 = v8::String::New("2");
   context_1->SetData(data_1);
   context_2->SetData(data_2);
   CHECK(context_1->GetData()->StrictEquals(data_1));
@@ -5233,7 +5233,7 @@ static void ExecuteScriptForContextCheck() {
   CHECK(context_1->GetData()->IsUndefined());
 
   // Set and check a data value.
-  v8::Handle<v8::Value> data_1 = v8::Number::New(1);
+  v8::Handle<v8::String> data_1 = v8::String::New("1");
   context_1->SetData(data_1);
   CHECK(context_1->GetData()->StrictEquals(data_1));
 
-- 
2.7.4