Bug 67727 - Make JSCell::toBoolean non-virtual
Summary: Make JSCell::toBoolean non-virtual
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
: 21703 (view as bug list)
Depends on: 68191
Blocks: 67690
  Show dependency treegraph
 
Reported: 2011-09-07 12:42 PDT by Mark Hahnenberg
Modified: 2011-09-26 16:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.98 KB, patch)
2011-09-08 11:49 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (7.09 KB, patch)
2011-09-08 14:07 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (9.59 KB, patch)
2011-09-09 13:00 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (9.69 KB, patch)
2011-09-09 14:20 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (9.65 KB, patch)
2011-09-09 18:08 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Windows? (9.58 KB, patch)
2011-09-12 11:52 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Benchmark perf comparison (6.71 KB, text/plain)
2011-09-12 16:43 PDT, Mark Hahnenberg
no flags Details
Actual benchmark results (6.63 KB, text/plain)
2011-09-12 17:14 PDT, Mark Hahnenberg
no flags Details
Patch (9.80 KB, patch)
2011-09-13 12:56 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (9.95 KB, patch)
2011-09-13 13:35 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (9.95 KB, patch)
2011-09-13 17:14 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (9.98 KB, patch)
2011-09-14 14:07 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (10.06 KB, patch)
2011-09-16 10:28 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (9.30 KB, patch)
2011-09-19 11:54 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Retrying (9.35 KB, patch)
2011-09-23 13:17 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2011-09-07 12:42:29 PDT
toBoolean is not used anywhere anymore.  We should remove it.
Comment 1 Mark Hahnenberg 2011-09-08 11:49:19 PDT
Created attachment 106770 [details]
Patch
Comment 2 Mark Hahnenberg 2011-09-08 14:07:41 PDT
Created attachment 106785 [details]
Patch
Comment 3 Mark Hahnenberg 2011-09-09 13:00:32 PDT
Created attachment 106908 [details]
Patch
Comment 4 Mark Hahnenberg 2011-09-09 14:20:24 PDT
Created attachment 106920 [details]
Patch
Comment 5 Geoffrey Garen 2011-09-09 17:13:22 PDT
Comment on attachment 106920 [details]
Patch

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

If you find any other implementations of toBoolean, I think you should remove those too. I believe there is at least one more you haven't removed yet.

> Source/JavaScriptCore/runtime/JSString.h:696
> +        if (isString())
> +            return static_cast<JSString*>(asCell())->toBoolean(exec);
> +        if (isCell())
> +            return !asCell()->structure()->typeInfo().masqueradesAsUndefined();
> +        return isTrue(); // false, null, and undefined all convert to false.

You could remove one branch by reordering here:

if (!isCell())
    return isTrue();
if (asCell()->isString())
    return static_cast<JSString*>(asCell())->isEmpty();
return asCell()->structure()->typeInfo().masqueradesAsUndefined();

> Source/JavaScriptCore/runtime/StringObjectThatMasqueradesAsUndefined.h:55
> -        virtual bool toBoolean(ExecState*) const { return false; }
> +        bool toBoolean(ExecState*) const { return false; }

This is no good -- JSValue already does the right thing and overriding non-virtual functions is not a good idea.
Comment 6 Darin Adler 2011-09-09 17:31:16 PDT
Comment on attachment 106920 [details]
Patch

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

>> Source/JavaScriptCore/runtime/StringObjectThatMasqueradesAsUndefined.h:55
>> +        bool toBoolean(ExecState*) const { return false; }
> 
> This is no good -- JSValue already does the right thing and overriding non-virtual functions is not a good idea.

I agree with Geoff that this override is not good, but overriding a non-virtual function can be useful in two ways:

    1) Writing a more efficient implementation to be used by callers that have a more-specific pointer. The more efficient implementation must have the same effects as the base class implementation to keep the design clean, but it’s good to get efficiency automatically out of the type system.

    2) Defining private overload of a function and not implementing it (or deleting it if we have a new enough compiler) to give a compiler or linker error if there is any accidental use of a too-general base class implementation. This catches a possible performance mistake at compile time.

In this case, I don’t think (1) is a good idea, and (2) is not helpful because it seems highly unlikely someone would call toBoolean on an object of this specific type by mistake. But I can imagine (2) being useful in JSCell and perhaps other classes derived from JSValue.
Comment 7 Mark Hahnenberg 2011-09-09 18:08:37 PDT
Created attachment 106948 [details]
Patch
Comment 8 Gavin Barraclough 2011-09-09 20:18:26 PDT
*** Bug 21703 has been marked as a duplicate of this bug. ***
Comment 9 Sam Weinig 2011-09-10 17:15:17 PDT
Comment on attachment 106948 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSString.h:697
> +    inline bool JSValue::toBoolean(ExecState* exec) const
> +    {
> +        if (isInt32())
> +            return asInt32();
> +        if (isDouble())
> +            return asDouble() > 0.0 || asDouble() < 0.0; // false for NaN
> +        if (!isCell())
> +            return isTrue(); // false, null, and undefined all convert to false.
> +        if (asCell()->isString())
> +            return static_cast<JSString*>(asCell())->toBoolean(exec);
> +        return !asCell()->structure()->typeInfo().masqueradesAsUndefined();
> +    }

Is there a real perf advantage to having this inline?
Comment 10 Oliver Hunt 2011-09-11 14:45:25 PDT
Comment on attachment 106948 [details]
Patch

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

r=me, assuming you fix the windows exports

> Source/JavaScriptCore/runtime/JSString.h:698
>  

In response to sam's comment: JSValue::toBoolean was already inline just in a different (random) file
Comment 11 Mark Hahnenberg 2011-09-12 11:52:57 PDT
Created attachment 107070 [details]
Windows?
Comment 12 Darin Adler 2011-09-12 15:15:15 PDT
I understand and approve of making JSCell::toBoolean non-virtual. I don’t understand doing that involved merging the contents of JSCell::toBoolean inside JSValue::toBoolean. If someone has a JSCell* and wants to call toBoolean on it, we don’t want to do the additional JSValue checks, so I would have kept JSValue::toBoolean calling the now-non-virtual JSCell.

Also, making all of JSValue::toBoolean inline is different from what we had before, since the JSCell::toBoolean part of it was not inline. It even cause a performance difference, either a speedup or slowdown, to do too much inlining here.
Comment 13 Mark Hahnenberg 2011-09-12 16:43:25 PDT
Created attachment 107112 [details]
Benchmark perf comparison

Here are some perf comparisons using the bencher script on the various benchmarks.  Seems to be a slight regression on kraken and not much of a change on v8 and sunspider.
Comment 14 Mark Hahnenberg 2011-09-12 16:48:34 PDT
(In reply to comment #13)
> Created an attachment (id=107112) [details]
> Benchmark perf comparison
> 
> Here are some perf comparisons using the bencher script on the various benchmarks.  Seems to be a slight regression on kraken and not much of a change on v8 and sunspider.

Ignore these results.  I ran the tests on the wrong tree :-/
Comment 15 Mark Hahnenberg 2011-09-12 17:14:22 PDT
Created attachment 107116 [details]
Actual benchmark results
Comment 16 Darin Adler 2011-09-12 17:16:14 PDT
Looks faster. Maybe we can make it even faster with further tuning, but clearly speed should not block checking in.
Comment 17 Mark Hahnenberg 2011-09-13 12:56:30 PDT
Created attachment 107214 [details]
Patch
Comment 18 Geoffrey Garen 2011-09-13 13:32:03 PDT
Comment on attachment 107214 [details]
Patch

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

Code looks good, but please update the ChangeLog to be congruent with the final patch you and Darin came up with here.

> Source/JavaScriptCore/ChangeLog:8
> +        JSCell::toBoolean has been removed and its descendents in JSObject and JSString

Not true anymore! :)
Comment 19 Mark Hahnenberg 2011-09-13 13:35:55 PDT
Created attachment 107220 [details]
Patch
Comment 20 Darin Adler 2011-09-13 14:44:28 PDT
Comment on attachment 107220 [details]
Patch

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

Looks great.

> Source/JavaScriptCore/runtime/JSString.h:702
> +        if (!isCell())
> +            return isTrue(); // false, null, and undefined all convert to false.
> +        return asCell()->toBoolean(exec);

The if statement here is reversed from the original in JSCell.h, and I’m not sure there is any reason to reverse it.
Comment 21 Mark Hahnenberg 2011-09-13 17:14:33 PDT
Created attachment 107264 [details]
Patch
Comment 22 Darin Adler 2011-09-13 17:26:11 PDT
Comment on attachment 107264 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        Its descendents in JSObject and JSString have also been made non-virtual.  JSValue now
> +        explicitly covers all cases of toBoolean, so having a virtual implementation of 

Typo: descendants

This should say JSCell, not JSValue.
Comment 23 Mark Hahnenberg 2011-09-14 14:07:46 PDT
Created attachment 107391 [details]
Patch
Comment 24 WebKit Review Bot 2011-09-14 22:15:39 PDT
Comment on attachment 107391 [details]
Patch

Clearing flags on attachment: 107391

Committed r95167: <http://trac.webkit.org/changeset/95167>
Comment 25 WebKit Review Bot 2011-09-14 22:15:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Darin Adler 2011-09-15 13:57:47 PDT
Comment on attachment 107391 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSString.h:65
> +        friend class JSCell;

We should probably roll this out and instead make the toBoolean function public. The reason it was private before no longer applies.

> Source/JavaScriptCore/runtime/JSString.h:67
> +        friend class JSValue;

We should probably roll this change back out. There is no need to have JSValue be a friend.
Comment 27 Mark Hahnenberg 2011-09-15 14:23:56 PDT
Rolled out patch to fix issues.
Comment 28 Mark Hahnenberg 2011-09-16 10:28:28 PDT
Created attachment 107677 [details]
Patch
Comment 29 WebKit Review Bot 2011-09-16 21:16:28 PDT
Comment on attachment 107677 [details]
Patch

Rejecting attachment 107677 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ore/runtime/JSNotAnObject.h
patching file Source/JavaScriptCore/runtime/JSObject.h
patching file Source/JavaScriptCore/runtime/JSString.h
patching file Source/JavaScriptCore/runtime/StringObjectThatMasqueradesAsUndefined.h
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file Source/JavaScriptCore/runtime/StringObjectThatMasqueradesAsUndefined.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9723405
Comment 30 Mark Hahnenberg 2011-09-19 11:54:49 PDT
Created attachment 107898 [details]
Patch
Comment 31 WebKit Review Bot 2011-09-21 13:28:14 PDT
Comment on attachment 107898 [details]
Patch

Rejecting attachment 107898 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
nObject.h
Hunk #1 succeeded at 65 with fuzz 2.
patching file Source/JavaScriptCore/runtime/JSObject.h
Hunk #1 succeeded at 133 with fuzz 2 (offset -1 lines).
patching file Source/JavaScriptCore/runtime/JSString.h
Hunk #1 FAILED at 426.
Hunk #2 succeeded at 494 with fuzz 2.
1 out of 3 hunks FAILED -- saving rejects to file Source/JavaScriptCore/runtime/JSString.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9770880
Comment 32 Mark Hahnenberg 2011-09-23 13:17:43 PDT
Created attachment 108522 [details]
Retrying
Comment 33 Geoffrey Garen 2011-09-23 13:35:41 PDT
Comment on attachment 108522 [details]
Retrying

r=me
Comment 34 WebKit Review Bot 2011-09-26 16:52:50 PDT
Comment on attachment 108522 [details]
Retrying

Clearing flags on attachment: 108522

Committed r96045: <http://trac.webkit.org/changeset/96045>
Comment 35 WebKit Review Bot 2011-09-26 16:52:57 PDT
All reviewed patches have been landed.  Closing bug.