use v8 Microtask Queue

Use v8 Microtask Queue

V8 now hosts the end-of-microtask work queue. This allows consistent
scheduling of microtask work regardless of where it is implemented (v8 vs blink).

R=adamk@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169978

[adamk, 2014-02-14 21:33:46]

haraken's probably more interested in this than abarth.

As noted below, I really would like to avoid these reinterpret_cast shenanigans.
Will do a bit of legwork...

https://codereview.chromium.org/167683003/diff/1/Source/core/dom/Microtask.cpp
File Source/core/dom/Microtask.cpp (right):

https://codereview.chromium.org/167683003/diff/1/Source/core/dom/Microtask.cp...
Source/core/dom/Microtask.cpp:60:
reinterpret_cast<MicrotaskCallback>(reinterpret_cast<intptr_t>(
This just looks worse and worse to me the more I see it. I'd like to
better-understand the C++ rule about void* and function callbacks before giving
this the OK in Blink. I'm going to see who I can rustle up (either ajwong or
jyasskin).

FWIW, here's what Chromium's base::Callback system has to say about this:

  // In C++, it is safe to cast function pointers to function pointers of
  // another type. It is not okay to use void*. We create a InvokeFuncStorage
  // that that can store our function pointer, and then cast it back to
  // the original type on usage.
  typedef void(*InvokeFuncStorage)(void);

I'm wondering if we could just get away with adding a new v8 type that does
something like this.

[adamk, 2014-02-14 22:26:37]

https://codereview.chromium.org/167683003/diff/1/Source/core/dom/Microtask.cpp
File Source/core/dom/Microtask.cpp (right):

https://codereview.chromium.org/167683003/diff/1/Source/core/dom/Microtask.cp...
Source/core/dom/Microtask.cpp:56: 
Please add a COMPILE_ASSERT that sizeof void* == sizeof MicrotaskCallback

Jeffrey suggests that that'll be sufficient to make this safe.

[rafaelw, 2014-02-14 22:52:51]

https://codereview.chromium.org/167683003/diff/1/Source/core/dom/Microtask.cpp
File Source/core/dom/Microtask.cpp (right):

https://codereview.chromium.org/167683003/diff/1/Source/core/dom/Microtask.cp...
Source/core/dom/Microtask.cpp:56: 
On 2014/02/14 22:26:37, adamk wrote:
> Please add a COMPILE_ASSERT that sizeof void* == sizeof MicrotaskCallback
> 
> Jeffrey suggests that that'll be sufficient to make this safe.

Done.

[adamk, 2014-02-14 23:26:22]

lgtm

https://codereview.chromium.org/167683003/diff/80001/Source/core/dom/Microtas...
File Source/core/dom/Microtask.cpp (right):

https://codereview.chromium.org/167683003/diff/80001/Source/core/dom/Microtas...
Source/core/dom/Microtask.cpp:59: COMPILE_ASSERT(sizeof(void*) ==
sizeof(MicrotaskCallback), VoidPtrAndFunctionPtrAreSameSize);
No need for two of these, we usually put COMPILE_ASSERTs at the file level.

[rafaelw, 2014-02-15 00:14:37]

https://codereview.chromium.org/167683003/diff/80001/Source/core/dom/Microtas...
File Source/core/dom/Microtask.cpp (right):

https://codereview.chromium.org/167683003/diff/80001/Source/core/dom/Microtas...
Source/core/dom/Microtask.cpp:59: COMPILE_ASSERT(sizeof(void*) ==
sizeof(MicrotaskCallback), VoidPtrAndFunctionPtrAreSameSize);
On 2014/02/14 23:26:22, adamk wrote:
> No need for two of these, we usually put COMPILE_ASSERTs at the file level.

Done.

[rafaelw, 2014-02-15 00:15:01]

The CQ bit was checked by rafaelw@chromium.org

[commit-bot: I haz the power, 2014-02-15 00:16:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rafaelw@chromium.org/167683003/130001

[commit-bot: I haz the power, 2014-02-15 02:06:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-15 02:06:39]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[rafaelw, 2014-02-21 02:24:03]

The CQ bit was checked by rafaelw@chromium.org

[commit-bot: I haz the power, 2014-02-21 02:24:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rafaelw@chromium.org/167683003/330001

[commit-bot: I haz the power, 2014-02-21 02:35:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 02:35:26]

Retried try job too often on mac_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[rafaelw, 2014-02-21 03:19:17]

The CQ bit was checked by rafaelw@chromium.org

[commit-bot: I haz the power, 2014-02-21 03:19:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rafaelw@chromium.org/167683003/330001

[commit-bot: I haz the power, 2014-02-21 04:44:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 04:44:41]

Retried try job too often on win_blink_rel for step(s) webkit_lint,
webkit_python_tests, webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[rafaelw, 2014-03-09 16:39:46]

The CQ bit was checked by rafaelw@chromium.org

[commit-bot: I haz the power, 2014-03-09 16:39:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rafaelw@chromium.org/167683003/330001

[commit-bot: I haz the power, 2014-03-09 16:39:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-09 16:39:59]

Failed to apply patch for Source/bindings/v8/V8PerIsolateData.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/v8/V8PerIsolateData.cpp
  Hunk #1 FAILED at 126.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/bindings/v8/V8PerIsolateData.cpp.rej

Patch:       Source/bindings/v8/V8PerIsolateData.cpp
Index: Source/bindings/v8/V8PerIsolateData.cpp
diff --git a/Source/bindings/v8/V8PerIsolateData.cpp
b/Source/bindings/v8/V8PerIsolateData.cpp
index
f9414337856b36169cbd76f49ba0cbf0ecd82d06..5894e967eb87a3ebd14bd9ede3971ee16bfa6b5b
100644
--- a/Source/bindings/v8/V8PerIsolateData.cpp
+++ b/Source/bindings/v8/V8PerIsolateData.cpp
@@ -126,13 +126,13 @@ void V8PerIsolateData::setDOMTemplate(WrapperWorldType
currentWorldType, void* d
     templateMap(currentWorldType).add(domTemplateKey,
UnsafePersistent<v8::FunctionTemplate>(m_isolate, templ));
 }
 
-v8::Local<v8::Context> V8PerIsolateData::ensureRegexContext()
+v8::Local<v8::Context> V8PerIsolateData::ensureDomInJSContext()
 {
-    if (m_regexContext.isEmpty()) {
+    if (m_domInJSContext.isEmpty()) {
         v8::HandleScope handleScope(m_isolate);
-        m_regexContext.set(m_isolate, v8::Context::New(m_isolate));
+        m_domInJSContext.set(m_isolate, v8::Context::New(m_isolate));
     }
-    return m_regexContext.newLocal(m_isolate);
+    return m_domInJSContext.newLocal(m_isolate);
 }
 
 bool V8PerIsolateData::hasInstanceInMainWorld(const WrapperTypeInfo* info,
v8::Handle<v8::Value> value)

[adamk, 2014-03-10 21:19:03]

I tried to land this myself in https://codereview.chromium.org/193123002/, but
ran into an assertion failure in v8 when running
inspector/sources/debugger/mutation-observer-suspend-while-paused.html:

#
# Fatal error in ../../v8/src/v8.cc, line 178
# CHECK(handle_scope_implementer->CallDepthIsZero()) failed
#

I suspect that this ASSERT is just wrong when V8::SetAutorunMicrotasks(false)
has been called, but I'd like to wait for rafaelw to return before making that
fix.

[rafaelw, 2014-03-24 20:10:13]

The CQ bit was checked by rafaelw@chromium.org

[commit-bot: I haz the power, 2014-03-24 20:10:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rafaelw@chromium.org/167683003/590001

[commit-bot: I haz the power, 2014-03-24 20:12:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-24 20:12:23]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[rafaelw, 2014-03-25 19:49:09]

Committed patchset #6 manually as r169978 (presubmit successful).

[tkent, 2014-03-27 10:14:17]

A revert of this CL has been created in
https://codereview.chromium.org/214563003/ by tkent@chromium.org.

The reason for reverting is: It seems this caused many memory leaks.

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%2BLSan%20Te...
.

[rafaelw1, 2014-03-27 17:16:35]

+jochen & yhirano

So I talked with abarth and he feels it's fine to leak this context, since
there's only one per process. So I propose to re-land this patch with
suppressions for asan for these specific leaks.

Everyone ok with that?


On Thu, Mar 27, 2014 at 3:14 AM, <tkent@chromium.org> wrote:

> A revert of this CL has been created in
> https://codereview.chromium.org/214563003/ by tkent@chromium.org.
>
> The reason for reverting is: It seems this caused many memory leaks.
>
> http://build.chromium.org/p/chromium.memory/builders/
> Linux%20ASan%2BLSan%20Tests%20%281%29/builds/807/steps/
> browser_tests/logs/stdio
> .
>
> https://codereview.chromium.org/167683003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[plz use chromium.org account, 2014-03-27 19:24:27]

Note that it's arbitrary many contexts per process being created by this,
but just one (the main thread one) being leaked.

so I agree, it's probably ok.


On Thu, Mar 27, 2014 at 6:16 PM, Rafael Weinstein <rafaelw@google.com>wrote:

> +jochen & yhirano
>
> So I talked with abarth and he feels it's fine to leak this context, since
> there's only one per process. So I propose to re-land this patch with
> suppressions for asan for these specific leaks.
>
> Everyone ok with that?
>
>
> On Thu, Mar 27, 2014 at 3:14 AM, <tkent@chromium.org> wrote:
>
>> A revert of this CL has been created in
>> https://codereview.chromium.org/214563003/ by tkent@chromium.org.
>>
>> The reason for reverting is: It seems this caused many memory leaks.
>>
>> http://build.chromium.org/p/chromium.memory/builders/
>> Linux%20ASan%2BLSan%20Tests%20%281%29/builds/807/steps/
>> browser_tests/logs/stdio
>> .
>>
>> https://codereview.chromium.org/167683003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[yhirano, 2014-03-28 09:15:57]

https://codereview.chromium.org/209853010/ which is landed now includes this CL.