toBoolean is not used anywhere anymore. We should remove it.
Created attachment 106770 [details] Patch
Created attachment 106785 [details] Patch
Created attachment 106908 [details] Patch
Created attachment 106920 [details] Patch
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 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.
Created attachment 106948 [details] Patch
*** Bug 21703 has been marked as a duplicate of this bug. ***
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 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
Created attachment 107070 [details] Windows?
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.
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.
(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 :-/
Created attachment 107116 [details] Actual benchmark results
Looks faster. Maybe we can make it even faster with further tuning, but clearly speed should not block checking in.
Created attachment 107214 [details] Patch
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! :)
Created attachment 107220 [details] Patch
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.
Created attachment 107264 [details] Patch
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.
Created attachment 107391 [details] Patch
Comment on attachment 107391 [details] Patch Clearing flags on attachment: 107391 Committed r95167: <http://trac.webkit.org/changeset/95167>
All reviewed patches have been landed. Closing bug.
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.
Rolled out patch to fix issues.
Created attachment 107677 [details] Patch
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
Created attachment 107898 [details] Patch
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
Created attachment 108522 [details] Retrying
Comment on attachment 108522 [details] Retrying r=me
Comment on attachment 108522 [details] Retrying Clearing flags on attachment: 108522 Committed r96045: <http://trac.webkit.org/changeset/96045>