Bug 49613 – [V8] ASSERT(WTF::isMainThread()) fails in V8Binding::int32ToWebCoreString in workers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

ASSERT(WTF::isMainThread()) fails in V8Binding::int32ToWebCoreString in workers.

In worker case, javascript is executed by a worker thread but not by the main thread so this assertion always fails.
For example, running the following test in chromium hits the assertion failure.
fast/workers/storage/execute-sql-args-worker.html

[Kinuko Yasuda]

Created attachment 74028 [details]
Proposing patch

[Nate Chapin]

Comment on attachment 74028 [details]
Proposing patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74028&action=review

Other than the style nits, this looks ok.  However, I'd appreciate it if you waited for antonm to agree before committing :)

> WebCore/bindings/v8/V8Binding.cpp:390
>      } else
> -        webCoreString = String::number(value);
> +      webCoreString = String::number(value);

Indent 4.

> WebCore/bindings/v8/V8Binding.cpp:399
> +    if (WTF::isMainThread())
> +      return int32ToWebCoreStringFast(value);

Indent 4.

[anton muhin]

Comment on attachment 74028 [details]
Proposing patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74028&action=review

> WebCore/bindings/v8/V8Binding.cpp:398
> +    if (WTF::isMainThread())

Please, check by running http://dromaeo.com/?dom performance implications.

I haven't looked into this code for some time, but WTF::isMainThread apparently used to be pretty expensive.

[Nate Chapin]

(In reply to comment #3)
> (From update of attachment 74028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74028&action=review
> 
> > WebCore/bindings/v8/V8Binding.cpp:398
> > +    if (WTF::isMainThread())
> 
> Please, check by running http://dromaeo.com/?dom performance implications.
> 
> I haven't looked into this code for some time, but WTF::isMainThread apparently used to be pretty expensive.

IIRC, globalObjectPrototypeIsDOMWindow() in V8DOMWrapper.cpp is a fairly cheap check for whether we're in a worker context or not.  Should we try to generalize that and put it somewhere public?

[Kinuko Yasuda]

I ran the dromaeo test with and w/o this patch (on my local linux box).
I didn't re-run the test more than twice for each, but the results looked ok/good to me, i.e. there doesn't seem a visible performance impact.

Before (w/o this patch):

http://dromaeo.com/?id=123148 
1441.23runs/s (Total)
http://dromaeo.com/?id=123179 
1418.11runs/s (Total)

After (with this patch):

http://dromaeo.com/?id=123150 
1437.06runs/s (Total)
http://dromaeo.com/?id=123177 
1426.23runs/s (Total)

[anton muhin]

Thank you very much, Kinuko!

(In reply to comment #5)
> I ran the dromaeo test with and w/o this patch (on my local linux box).
> I didn't re-run the test more than twice for each, but the results looked ok/good to me, i.e. there doesn't seem a visible performance impact.
> 
> Before (w/o this patch):
> 
> http://dromaeo.com/?id=123148 
> 1441.23runs/s (Total)
> http://dromaeo.com/?id=123179 
> 1418.11runs/s (Total)
> 
> After (with this patch):
> 
> http://dromaeo.com/?id=123150 
> 1437.06runs/s (Total)
> http://dromaeo.com/?id=123177 
> 1426.23runs/s (Total)

[Kinuko Yasuda]

Committed r72354: <http://trac.webkit.org/changeset/72354>