[console] fast console.assert(true)

[console] fast console.assert(true)

A lot of web sites around the world has hack which replaces native console.assert by function with fast return.
Current console.assert is slow because we need to run CPP builtin but we should enter this builtin iff condition is false or omitted.

BUG=v8:6175
R=ishell@chromium.org,dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2828933002
Cr-Commit-Position: refs/heads/master@{#44752}
Committed: https://chromium.googlesource.com/v8/v8/+/f28e487858ef9048bcd8bc4a1bffa0ff8770bf57

[kozy, 2017-04-19 22:01:36]

Dmitry and Igor, please take a look.

This implementation is ~10 times faster for console.assert(true) but I've a
question for Igor.

If I implement console.assert by myself:
function assert(check, arg1, arg2, arg3, arg4, arg5, arg6) {
return !check && console.error(arg1, arg2, arg3, arg4, arg5, arg6);
}
it's still much faster (~5 times). Is it because this function doesn't support
arbitrary number of arguments or because it's inlined if I'm running it in the
cycle? Can I somehow use knowledge that I'd like to check only first argument to
speedup implementation?

[Igor Sheludko, 2017-04-20 10:18:42]

lgtm with nits.


I think manual implementation is faster because of inlining and constant
propagation, so the whole overhead of a call is gone (no need to push arguments
on stack, etc.). You can try your code with --trace-turbo-inlining and
--trace-inlining to check our hypothesis or just disable inlining.
You can also try to pollute type feedback for ToBoolean(check) IC by passing
various values as "check" argument.


I think the "true" case of the TFJ stub is already good enough: it does not
require arguments adapter frame and it can get to the first argument only in 4
instructions.

https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
File src/builtins/builtins-console-gen.cc (right):

https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-console-gen.cc:23: GotoIf(WordEqual(argc,
IntPtrConstant(0)), &runtime);
argc is an int32, so the graph verifier will complain about operand types
mismatch.

I think you can remove this line and add
  GotoIf(Word32Equal(argc, Int32Constant(0)), &runtime);
before line 22.

https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-console-gen.cc:26: {
args.PopAndReturn(UndefinedConstant()); }
Please remove {} and add an empty line after this line.

https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-console-gen.cc:31:
TailCallStub(CodeFactory::ConsoleAssert(isolate()), context, target,
It's simplier to use TailCallBuiltin().

https://codereview.chromium.org/2828933002/diff/20001/src/code-factory.cc
File src/code-factory.cc (right):

https://codereview.chromium.org/2828933002/diff/20001/src/code-factory.cc#new...
src/code-factory.cc:448: Callable CodeFactory::ConsoleAssert(Isolate* isolate) {
Same here.

https://codereview.chromium.org/2828933002/diff/20001/src/code-factory.h
File src/code-factory.h (right):

https://codereview.chromium.org/2828933002/diff/20001/src/code-factory.h#newc...
src/code-factory.h:169: static Callable ConsoleAssert(Isolate* isolate);
It will not be necessary if you use CSA::TailCallBuiltin().

https://codereview.chromium.org/2828933002/diff/20001/src/inspector/v8-consol...
File src/inspector/v8-console.cc (right):

https://codereview.chromium.org/2828933002/diff/20001/src/inspector/v8-consol...
src/inspector/v8-console.cc:274: void V8Console::Assert(const
v8::debug::ConsoleCallArguments& info) {
Maybe ReportAssertionFailure or something similar will be a better name for this
builtin.

https://codereview.chromium.org/2828933002/diff/20001/src/inspector/v8-consol...
src/inspector/v8-console.cc:275: ConsoleHelper helper(info, m_inspector);
or DCHECK(!helper.firstArgToBoolean(false));

[Igor Sheludko, 2017-04-20 14:26:11]

https://codereview.chromium.org/2828933002/diff/20001/src/inspector/v8-consol...
File src/inspector/v8-console.cc (right):

https://codereview.chromium.org/2828933002/diff/20001/src/inspector/v8-consol...
src/inspector/v8-console.cc:274: void V8Console::Assert(const
v8::debug::ConsoleCallArguments& info) {
On 2017/04/20 10:18:42, Igor Sheludko wrote:
> Maybe ReportAssertionFailure or something similar will be a better name for
this
> builtin.

Given that we are passing all the arguments (including the "check" value) the
Assert name is fine. Please ignore the above comment.

[kozy, 2017-04-20 15:14:55]

thanks! please take another look, I've some questions.

https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
File src/builtins/builtins-console-gen.cc (right):

https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-console-gen.cc:23: GotoIf(WordEqual(argc,
IntPtrConstant(0)), &runtime);
On 2017/04/20 10:18:42, Igor Sheludko wrote:
> argc is an int32, so the graph verifier will complain about operand types
> mismatch.
> 
> I think you can remove this line and add
>   GotoIf(Word32Equal(argc, Int32Constant(0)), &runtime);
> before line 22.

Done.

https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-console-gen.cc:26: {
args.PopAndReturn(UndefinedConstant()); }
On 2017/04/20 10:18:42, Igor Sheludko wrote:
> Please remove {} and add an empty line after this line.

Done.

https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-console-gen.cc:31:
TailCallStub(CodeFactory::ConsoleAssert(isolate()), context, target,
On 2017/04/20 10:18:42, Igor Sheludko wrote:
> It's simplier to use TailCallBuiltin().

I couldn't find TailCallBuiltin in code-assembler, there is one located in
access-compiler.h but it looks related to MasmAssembler.
There is TailCallRuntime but I think that builtin and runtime functions have
different call descriptors, haven't they?

https://codereview.chromium.org/2828933002/diff/20001/src/inspector/v8-consol...
File src/inspector/v8-console.cc (right):

https://codereview.chromium.org/2828933002/diff/20001/src/inspector/v8-consol...
src/inspector/v8-console.cc:274: void V8Console::Assert(const
v8::debug::ConsoleCallArguments& info) {
On 2017/04/20 14:26:10, Igor Sheludko wrote:
> On 2017/04/20 10:18:42, Igor Sheludko wrote:
> > Maybe ReportAssertionFailure or something similar will be a better name for
> this
> > builtin.
> 
> Given that we are passing all the arguments (including the "check" value) the
> Assert name is fine. Please ignore the above comment.

It would be cool to pass all arguments except first one but as far as I
understand to avoid any kind of hacks and introducing new CallDescriptor for
specific ConsoleAssert it's not possible without popping all arguments and then
pushing back all of them except first one.

https://codereview.chromium.org/2828933002/diff/20001/src/inspector/v8-consol...
src/inspector/v8-console.cc:275: ConsoleHelper helper(info, m_inspector);
On 2017/04/20 10:18:42, Igor Sheludko wrote:
> or DCHECK(!helper.firstArgToBoolean(false));

added DCHECK.

[Igor Sheludko, 2017-04-20 15:20:10]

https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
File src/builtins/builtins-console-gen.cc (right):

https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-console-gen.cc:31:
TailCallStub(CodeFactory::ConsoleAssert(isolate()), context, target,
On 2017/04/20 15:14:54, kozy wrote:
> On 2017/04/20 10:18:42, Igor Sheludko wrote:
> > It's simplier to use TailCallBuiltin().
> 
> I couldn't find TailCallBuiltin in code-assembler, there is one located in
> access-compiler.h but it looks related to MasmAssembler.
> There is TailCallRuntime but I think that builtin and runtime functions have
> different call descriptors, haven't they?

I'm sorry. Please add CSA::TailCallBuiltin(), see CSA::CallBuiltin() for a
reference.

[kozy, 2017-04-20 15:34:34]

Patchset #4 (id:60001) has been deleted

[kozy, 2017-04-20 15:36:46]

On 2017/04/20 15:20:10, Igor Sheludko wrote:
>
https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
> File src/builtins/builtins-console-gen.cc (right):
> 
>
https://codereview.chromium.org/2828933002/diff/20001/src/builtins/builtins-c...
> src/builtins/builtins-console-gen.cc:31:
> TailCallStub(CodeFactory::ConsoleAssert(isolate()), context, target,
> On 2017/04/20 15:14:54, kozy wrote:
> > On 2017/04/20 10:18:42, Igor Sheludko wrote:
> > > It's simplier to use TailCallBuiltin().
> > 
> > I couldn't find TailCallBuiltin in code-assembler, there is one located in
> > access-compiler.h but it looks related to MasmAssembler.
> > There is TailCallRuntime but I think that builtin and runtime functions have
> > different call descriptors, haven't they?
> 
> I'm sorry. Please add CSA::TailCallBuiltin(), see CSA::CallBuiltin() for a
> reference.

Added, but I added CPP builtins to the switch in Builtins::CallableFor to
support CPP ConsoleAssert builtin.

[kozy, 2017-04-20 15:36:51]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 15:36:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Igor Sheludko, 2017-04-20 15:55:19]

lgtm

https://codereview.chromium.org/2828933002/diff/80001/src/builtins/builtins.cc
File src/builtins/builtins.cc (right):

https://codereview.chromium.org/2828933002/diff/80001/src/builtins/builtins.c...
src/builtins/builtins.cc:131: return Callable(code,
ConstructTrampolineDescriptor(isolate)); \
s/ConstructTrampolineDescriptor/BuiltinDescriptor/

[commit-bot: I haz the power, 2017-04-20 16:04:04]

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

[commit-bot: I haz the power, 2017-04-20 16:04:05]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-04-20 16:48:15]

thanks.

https://codereview.chromium.org/2828933002/diff/80001/src/builtins/builtins.cc
File src/builtins/builtins.cc (right):

https://codereview.chromium.org/2828933002/diff/80001/src/builtins/builtins.c...
src/builtins/builtins.cc:131: return Callable(code,
ConstructTrampolineDescriptor(isolate)); \
On 2017/04/20 15:55:19, Igor Sheludko wrote:
> s/ConstructTrampolineDescriptor/BuiltinDescriptor/

Done.

[kozy, 2017-04-20 16:48:18]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-04-20 16:48:19]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2828933002/#ps100001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-04-20 16:48:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 17:17:19]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492706898772830,
"parent_rev": "d089276e0383a35f6cc74fdf8509c0afa9e05a83", "commit_rev":
"f28e487858ef9048bcd8bc4a1bffa0ff8770bf57"}

[commit-bot: I haz the power, 2017-04-20 17:17:26]

Description was changed from

==========
[console] fast console.assert(true)

A lot of web sites around the world has hack which replaces native
console.assert by function with fast return.
Current console.assert is slow because we need to run CPP builtin but we should
enter this builtin iff condition is false or omitted.

BUG=v8:6175
R=ishell@chromium.org,dgozman@chromium.org
==========

to

==========
[console] fast console.assert(true)

A lot of web sites around the world has hack which replaces native
console.assert by function with fast return.
Current console.assert is slow because we need to run CPP builtin but we should
enter this builtin iff condition is false or omitted.

BUG=v8:6175
R=ishell@chromium.org,dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2828933002
Cr-Commit-Position: refs/heads/master@{#44752}
Committed:
https://chromium.googlesource.com/v8/v8/+/f28e487858ef9048bcd8bc4a1bffa0ff877...
==========

[commit-bot: I haz the power, 2017-04-20 17:17:27]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/f28e487858ef9048bcd8bc4a1bffa0ff877...