[compiler-dispatcher] make it so that we can always parse on bg threads

src/compiler-dispatcher/compiler-dispatcher-job.cc

Index: src/compiler-dispatcher/compiler-dispatcher-job.cc
diff --git a/src/compiler-dispatcher/compiler-dispatcher-job.cc b/src/compiler-dispatcher/compiler-dispatcher-job.cc
index 2ca26011bb5646dbacef9b725ac015d2de52af5c..fdb975a5e419fde3427a53514cc5a8730ec33694 100644
--- a/src/compiler-dispatcher/compiler-dispatcher-job.cc
+++ b/src/compiler-dispatcher/compiler-dispatcher-job.cc
@@ -22,6 +22,46 @@
 namespace v8 {
 namespace internal {
 
+namespace {
+
+class OneByteWrapper : public v8::String::ExternalOneByteStringResource {
+ public:
+  OneByteWrapper(const void* data, int length) : data_(data), length_(length) {}
+  ~OneByteWrapper() override = default;
+
+  const char* data() const override {
+    return reinterpret_cast<const char*>(data_);
+  }
+
+  size_t length() const override { return static_cast<size_t>(length_); }
+
+ private:
+  const void* data_;
+  int length_;
+
+  DISALLOW_COPY_AND_ASSIGN(OneByteWrapper);
+};
+
+class TwoByteWrapper : public v8::String::ExternalStringResource {
+ public:
+  TwoByteWrapper(const void* data, int length) : data_(data), length_(length) {}
+  ~TwoByteWrapper() override = default;
+
+  const uint16_t* data() const override {
+    return reinterpret_cast<const uint16_t*>(data_);
+  }
+
+  size_t length() const override { return static_cast<size_t>(length_); }
+
+ private:
+  const void* data_;
+  int length_;
+
+  DISALLOW_COPY_AND_ASSIGN(TwoByteWrapper);
+};
+
+}  // namespace
+
 CompilerDispatcherJob::CompilerDispatcherJob(Isolate* isolate,
                                              CompilerDispatcherTracer* tracer,
                                              Handle<SharedFunctionInfo> shared,
@@ -31,14 +71,11 @@ CompilerDispatcherJob::CompilerDispatcherJob(Isolate* isolate,
       shared_(Handle<SharedFunctionInfo>::cast(
           isolate_->global_handles()->Create(*shared))),
       max_stack_size_(max_stack_size),
-      can_compile_on_background_thread_(false),
       trace_compiler_dispatcher_jobs_(FLAG_trace_compiler_dispatcher_jobs) {
   HandleScope scope(isolate_);
   DCHECK(!shared_->outer_scope_info()->IsTheHole(isolate_));
   Handle<Script> script(Script::cast(shared_->script()), isolate_);
   Handle<String> source(String::cast(script->source()), isolate_);
-  can_parse_on_background_thread_ =
-      source->IsExternalTwoByteString() || source->IsExternalOneByteString();
   if (trace_compiler_dispatcher_jobs_) {
     PrintF("CompilerDispatcherJob[%p] created for ", static_cast<void*>(this));
     shared_->ShortPrint();
@@ -78,11 +115,68 @@ void CompilerDispatcherJob::PrepareToParseOnMainThread() {
         source, shared_->start_position(), shared_->end_position()));
   } else {
     source = String::Flatten(source);

[vogelheim, 2017/01/13 15:20:22]

[all below:]

I'm vaguely uncomfortable with this whole logic, since it (potentially)
introduces a new copy, while we already have so many copies of the source string
that we're desperately trying to get rid of some.

I take it this should rarely (if ever) be triggered for 'real' code, since
pretty much anything we get from Blink is an External(One|Two)ByteString? If so,
my concern is still there, but more academic.


Since at the current state I don't really have an alternative I don#t want to
hold this up. I do suspect we can do better by looking at, and I do think we
need to revisit this at some point...

[jochen (gone - plz use gerrit), 2017/01/13 16:14:40]

Right. I'm also trying to reduce the impact by just copying the function, not
the entire string (if I have to copy)

-    // Have to globalize the reference here, so it survives between function
-    // calls.
-    source_ = Handle<String>::cast(isolate_->global_handles()->Create(*source));
-    character_stream_.reset(ScannerStream::For(
-        source_, shared_->start_position(), shared_->end_position()));
+    const void* data;
+    int offset = 0;
+    int length = source->length();
+
+    // Objects in lo_space don't move, so we can just read the contents from
+    // any thread.
+    if (isolate_->heap()->lo_space()->Contains(*source)) {

[jochen (gone - plz use gerrit), 2017/01/13 13:21:39]

Hannes, ptal at this if () {} block: here I'm depending on the fact that strings
in LO space won't move, so I can parse off of them on bg threads.

[Hannes Payer (out of office), 2017/01/13 14:01:04]

Correct, LO don't move.

[rmcilroy, 2017/01/13 15:56:22]

Is it common for the source to be in the LO space? I'm guessing the source is
usually an external string, and would only be on the heap for eval functions (am
I missing other situations?), and therefore would only be in the LO space if
it's a 4KB eval - I'm guessing this isn't super common - is it worth the added
complexity here?

[jochen (gone - plz use gerrit), 2017/01/13 16:14:40]

no, I expect most cases to be external strings, or small evals. However, if
somebody implements a module loading using eval, we might hit this.

I think it's preferable to just always be able to run on bg threads

+      // We need to globalize the handle to the flattened string here, in
+      // case it's not referenced from anywhere else.
+      source_ =
+          Handle<String>::cast(isolate_->global_handles()->Create(*source));
+      DisallowHeapAllocation no_allocation;
+      String::FlatContent content = source->GetFlatContent();
+      DCHECK(content.IsFlat());
+      data =
+          content.IsOneByte()
+              ? reinterpret_cast<const void*>(content.ToOneByteVector().start())
+              : reinterpret_cast<const void*>(content.ToUC16Vector().start());
+    } else {
+      // Otherwise, create a copy of the part of the string we'll parse in the
+      // zone.
+      length = (shared_->end_position() - shared_->start_position());
+      offset = shared_->start_position();
+
+      int byte_len = length * (source->IsOneByteRepresentation() ? 1 : 2);
+      data = zone_->New(byte_len);

[vogelheim, 2017/01/13 15:20:22]

Who will delete this?

As I see it, it get pointed to from an External(One|Two)ByteString, but I
thought those typically don't delete their memory (as they don't normally own
it), so... where does this get deleted?

[jochen (gone - plz use gerrit), 2017/01/13 16:14:40]

the zone_ will delete this

+
+      DisallowHeapAllocation no_allocation;
+      String::FlatContent content = source->GetFlatContent();
+      DCHECK(content.IsFlat());
+      if (content.IsOneByte()) {
+        MemCopy(const_cast<void*>(data),
+                &content.ToOneByteVector().at(shared_->start_position()),
+                byte_len);
+      } else {
+        MemCopy(const_cast<void*>(data),
+                &content.ToUC16Vector().at(shared_->start_position()),
+                byte_len);
+      }
+    }
+    Handle<String> wrapper;
+    if (source->IsOneByteRepresentation()) {
+      ExternalOneByteString::Resource* resource =
+          new OneByteWrapper(data, length);
+      source_wrapper_.reset(resource);
+      wrapper = isolate_->factory()
+                    ->NewExternalStringFromOneByte(resource)
+                    .ToHandleChecked();
+    } else {
+      ExternalTwoByteString::Resource* resource =
+          new TwoByteWrapper(data, length);
+      source_wrapper_.reset(resource);
+      wrapper = isolate_->factory()
+                    ->NewExternalStringFromTwoByte(resource)
+                    .ToHandleChecked();
+    }
+    wrapper_ =
+        Handle<String>::cast(isolate_->global_handles()->Create(*wrapper));
+
+    character_stream_.reset(
+        ScannerStream::For(wrapper_, shared_->start_position() - offset,
+                           shared_->end_position() - offset));
   }
   parse_info_.reset(new ParseInfo(zone_.get()));
   parse_info_->set_isolate(isolate_);
@@ -111,8 +205,6 @@ void CompilerDispatcherJob::PrepareToParseOnMainThread() {
 }
 
 void CompilerDispatcherJob::Parse() {
-  DCHECK(can_parse_on_background_thread_ ||
-         ThreadId::Current().Equals(isolate_->thread_id()));
   DCHECK(status() == CompileJobStatus::kReadyToParse);
   COMPILER_DISPATCHER_TRACE_SCOPE_WITH_NUM(
       tracer_, kParse,
@@ -123,12 +215,7 @@ void CompilerDispatcherJob::Parse() {
 
   DisallowHeapAllocation no_allocation;
   DisallowHandleAllocation no_handles;
-  std::unique_ptr<DisallowHandleDereference> no_deref;
-  // If we can't parse on a background thread, we need to be able to deref the
-  // source string.
-  if (can_parse_on_background_thread_) {
-    no_deref.reset(new DisallowHandleDereference());
-  }
+  DisallowHandleDereference no_deref;
 
   // Nullify the Isolate temporarily so that the parser doesn't accidentally
   // use it.
@@ -157,6 +244,10 @@ bool CompilerDispatcherJob::FinalizeParsingOnMainThread() {
     i::GlobalHandles::Destroy(Handle<Object>::cast(source_).location());
     source_ = Handle<String>::null();
   }
+  if (!wrapper_.is_null()) {
+    i::GlobalHandles::Destroy(Handle<Object>::cast(wrapper_).location());
+    wrapper_ = Handle<String>::null();
+  }
 
   if (parse_info_->literal() == nullptr) {
     status_ = CompileJobStatus::kFailed;
@@ -217,16 +308,13 @@ bool CompilerDispatcherJob::PrepareToCompileOnMainThread() {
     return false;
   }
 
-  can_compile_on_background_thread_ =
-      compile_job_->can_execute_on_background_thread();
+  CHECK(compile_job_->can_execute_on_background_thread());
   status_ = CompileJobStatus::kReadyToCompile;
   return true;
 }
 
 void CompilerDispatcherJob::Compile() {
   DCHECK(status() == CompileJobStatus::kReadyToCompile);
-  DCHECK(can_compile_on_background_thread_ ||
-         ThreadId::Current().Equals(isolate_->thread_id()));
   COMPILER_DISPATCHER_TRACE_SCOPE_WITH_NUM(
       tracer_, kCompile, parse_info_->literal()->ast_node_count());
   if (trace_compiler_dispatcher_jobs_) {
@@ -293,6 +381,10 @@ void CompilerDispatcherJob::ResetOnMainThread() {
     i::GlobalHandles::Destroy(Handle<Object>::cast(source_).location());
     source_ = Handle<String>::null();
   }
+  if (!wrapper_.is_null()) {
+    i::GlobalHandles::Destroy(Handle<Object>::cast(wrapper_).location());
+    wrapper_ = Handle<String>::null();
+  }
 
   status_ = CompileJobStatus::kInitial;
 }