Bug 157297 - Web Inspector: console.assert should do far less work when the assertion is true
Summary: Web Inspector: console.assert should do far less work when the assertion is true
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-02 20:50 PDT by Joseph Pecoraro
Modified: 2016-05-03 08:32 PDT (History)
11 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (3.62 KB, patch)
2016-05-02 20:55 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-05-02 20:50:36 PDT
* SUMMARY
console.assert should do far less work when the assertion is true.

* TEST
<script>
(function() {    
    function bench(name, f, arg) {
        var warmup = f();
        var start = Date.now();
        var result = f();
        var end = Date.now();
        console.log(name, (end-start)+"ms");
    }

    bench("console.assert...", function() {
        for (var i = 0; i < 1e6; ++i)
            console.assert(true, "message");
    });
})();
</script>

* RESULTS
• Safari - console.assert... – "119ms"
• Chrome - console.assert... 22ms
• Firefox - console.assert... 44ms

We do a lot of work in the case where the assert doesn't fire. After a quick fix:
• Safari - console.assert... – "9ms"
Comment 1 Radar WebKit Bug Importer 2016-05-02 20:52:01 PDT
<rdar://problem/26056556>
Comment 2 Joseph Pecoraro 2016-05-02 20:55:19 PDT
Created attachment 277972 [details]
[PATCH] Proposed Fix

We could make assert an intrinsic in the future, but I'm not sure it really needs it. This alone was a 10x speedup.
Comment 3 Timothy Hatcher 2016-05-02 21:52:29 PDT
Comment on attachment 277972 [details]
[PATCH] Proposed Fix

Nice!
Comment 4 WebKit Commit Bot 2016-05-03 08:24:00 PDT
Comment on attachment 277972 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 277972

Committed r200371: <http://trac.webkit.org/changeset/200371>
Comment 5 WebKit Commit Bot 2016-05-03 08:24:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2016-05-03 08:32:36 PDT
Comment on attachment 277972 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/runtime/ConsoleClient.h:53
> +    void assertion(ExecState*, RefPtr<Inspector::ScriptArguments>&&);

The right name for this seems to be "assertionFailed".

Longer term, really puzzled about the fact that ScriptArguments is a reference counted object.