Bug 67727 – Make JSCell::toBoolean non-virtual

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

toBoolean is not used anywhere anymore.  We should remove it.

[Mark Hahnenberg]

Created attachment 106770 [details]
Patch

[Mark Hahnenberg]

Created attachment 106785 [details]
Patch

[Mark Hahnenberg]

Created attachment 106908 [details]
Patch

[Mark Hahnenberg]

Created attachment 106920 [details]
Patch

[Geoffrey Garen]

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.

[Darin Adler]

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.

[Mark Hahnenberg]

Created attachment 106948 [details]
Patch

[Gavin Barraclough]

*** Bug 21703 has been marked as a duplicate of this bug. ***

[Sam Weinig]

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?

[Oliver Hunt]

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

[Mark Hahnenberg]

Created attachment 107070 [details]
Windows?

[Darin Adler]

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.

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

(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 :-/

[Mark Hahnenberg]

Created attachment 107116 [details]
Actual benchmark results

[Darin Adler]

Looks faster. Maybe we can make it even faster with further tuning, but clearly speed should not block checking in.

[Mark Hahnenberg]

Created attachment 107214 [details]
Patch

[Geoffrey Garen]

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! :)

[Mark Hahnenberg]

Created attachment 107220 [details]
Patch

[Darin Adler]

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.

[Mark Hahnenberg]

Created attachment 107264 [details]
Patch

[Darin Adler]

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.

[Mark Hahnenberg]

Created attachment 107391 [details]
Patch

[WebKit Review Bot]

Comment on attachment 107391 [details]
Patch

Clearing flags on attachment: 107391

Committed r95167: <http://trac.webkit.org/changeset/95167>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Mark Hahnenberg]

Rolled out patch to fix issues.

[Mark Hahnenberg]

Created attachment 107677 [details]
Patch

[WebKit Review Bot]

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

[Mark Hahnenberg]

Created attachment 107898 [details]
Patch

[WebKit Review Bot]

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

[Mark Hahnenberg]

Created attachment 108522 [details]
Retrying

[Geoffrey Garen]

Comment on attachment 108522 [details]
Retrying

r=me

[WebKit Review Bot]

Comment on attachment 108522 [details]
Retrying

Clearing flags on attachment: 108522

Committed r96045: <http://trac.webkit.org/changeset/96045>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.