New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 663845 link

Starred by 1 user
Status: Fixed
Owner:
█ kozyatinskiy@chromium.org
Last visit > 30 days ago
Closed: Nov 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

DevTools: console.assert is slow

Project Member Reported by lushnikov@chromium.org, Nov 9 2016 
Today, we employ the following hack for console.assert in utilities.js:

// FIXME: This performance optimization should be moved to blink so that all developers could enjoy it.
// console is retrieved with V8Window.getAttribute method which is slow. Here we copy it to a js variable for faster access.
console = console;
console.__originalAssert = console.assert;
console.assert = function(value, message) {
  if (value)
    return;
  console.__originalAssert(value, message);
};

It looks like it should be easy to finally address this FIXME. What do you think?

Comment 1 by kozyatinskiy@chromium.org, Nov 10 2016 

Do you have any evidence that console.assert is slow in ToT?
I think that it's already fixed, I'll check it.
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 17 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f48b5bcde47df6852eb72e5f4be3a21f149e0d0

commit 7f48b5bcde47df6852eb72e5f4be3a21f149e0d0
Author: kozyatinskiy <kozyatinskiy@chromium.org>
Date: Thu Nov 17 18:38:02 2016

[DevTools] contextCreated should be called when context initialization is done

Inspector can evaluate and run debugger internal scripts in contextCreated method.

BUG= chromium:663845 
R=dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2498213002
Cr-Commit-Position: refs/heads/master@{#432923}

[modify] https://crrev.com/7f48b5bcde47df6852eb72e5f4be3a21f149e0d0/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp

Comment 3 Deleted

Comment 4 by kozyatinskiy@chromium.org, Nov 22 2016 

With CL: https://codereview.chromium.org/2505493002/
from jsperf:
builtin console implementation is faster then custom one and even faster then "if (false) console.error(..)":)
Screen Shot 2016-11-21 at 22.38.09.png
690 KB View Download

Comment 5 by kozyatinskiy@chromium.org, Nov 22 2016 

Attached jsperf without patch. New implementation is around twice faster for console.assert(true) and ~20% faster for console.assert(false).
Screenshot from 2016-11-22 09:09:46.png
60.3 KB View Download
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 23 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c1622945f71d52817568b6f830d138914b604d8a

commit c1622945f71d52817568b6f830d138914b604d8a
Author: machenbach <machenbach@chromium.org>
Date: Wed Nov 23 15:53:43 2016

Revert of [inspector] make console.assert much faster (patchset #3 id:40001 of https://codereview.chromium.org/2505493002/ )

Reason for revert:
Speculative revert as there seems to be a layout test crash:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/11585

Please reland if it doesn't get green.

Original issue's description:
> [inspector] make console.assert much faster
>
> New console.assert implementation is faster then custom user implementation.
>
> BUG= chromium:663845 
> R=dgozman@chromium.org
>
> Committed: https://crrev.com/f658e41d864267fb9e99ea76faa7758b0b63d5c9
> Cr-Commit-Position: refs/heads/master@{#41227}

TBR=dgozman@chromium.org,yangguo@chromium.org,kozyatinskiy@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:663845 

Review-Url: https://codereview.chromium.org/2521323005
Cr-Commit-Position: refs/heads/master@{#41231}

[modify] https://crrev.com/c1622945f71d52817568b6f830d138914b604d8a/src/inspector/v8-console.cc
[modify] https://crrev.com/c1622945f71d52817568b6f830d138914b604d8a/test/debugger/debug/debug-script.js
[delete] https://crrev.com/1a4294b1811837e07cb5a9e8db0c2826f744aa9c/test/inspector/runtime/console-assert-expected.txt
[delete] https://crrev.com/1a4294b1811837e07cb5a9e8db0c2826f744aa9c/test/inspector/runtime/console-assert.js
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 29 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b66d8ae9dc168c81845b28f36fcc9ae27522afd2

commit b66d8ae9dc168c81845b28f36fcc9ae27522afd2
Author: kozyatinskiy <kozyatinskiy@chromium.org>
Date: Tue Nov 29 21:35:03 2016

[DevTools] remove old console.assert hack

New console.assert implementation is faster then any hacks.

BUG= chromium:663845 
R=lushnikov@chromium.org

Review-Url: https://codereview.chromium.org/2525543002
Cr-Commit-Position: refs/heads/master@{#435081}

[modify] https://crrev.com/b66d8ae9dc168c81845b28f36fcc9ae27522afd2/third_party/WebKit/Source/devtools/front_end/platform/utilities.js

Comment 10 by kozyatinskiy@chromium.org, Nov 29 2016

Status: Fixed (was: Assigned)
►

Sign in to add a comment