eliminate JSC::NaN and JSC::Inf

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Darin Adler]

There's no good reason for KJS to have its own NAN and infinity constants. The ones in std::numeric_limits are perfectly good.

I have a patch to get rid of them.

[Darin Adler]

Created attachment 18321 [details]
patch -- I'd land it if SunSpider didn't seem to slow down by 0.5%

[David Kilzer (:ddkilzer)]

Is there a way to replace the value of KJS::NaN and KJS::Inf with numeric_limits<double>::quiet_NaN() and numeric_limits<double>::infinity() (respectively) without creating a static initializer?

[Darin Adler]

(In reply to comment #2)
> Is there a way to replace the value of KJS::NaN and KJS::Inf with
> numeric_limits<double>::quiet_NaN() and numeric_limits<double>::infinity()
> (respectively) without creating a static initializer?

I don't think so.

But that's a problem we have already solved for platforms that define NAN and INFINITY, which takes care of OS X. So I'm not really concerned about it. This bug is more about cleaning things up so we don't have our own NaN and infinity constants at all, which seems unnecessary since the standard library already has them.

My guess is that almost all call sites could be changed as in this patch and then we can figure out why the one call site is so hot. Once we understand that we might be able to come up with an even faster way to do it -- for example, we could store the value in the ExecState structure.

[Gavin Barraclough]

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

[Gavin Barraclough]

From 19272:

From Darin Adler 2008-05-27 11:51:16 PST (-) [reply]
JavaScriptCore uses the following redundant ways to get the double NAN:

    function std::numeric_limits<double>::quiet_NaN()
    global KJS::NaN
    macro NAN

There's also code that says (isnan || isinf) where it could more efficiently say !isfinite instead. We should clean this up. I'd like to see KJS::NaN and KJS::Inf go away entirely.

[Gavin Barraclough]

Created attachment 96833 [details]
The patch (no perf impact)

[Gavin Barraclough]

Fixed in r88587

[Darin Adler]

Comment on attachment 96833 [details]
The patch (no perf impact)

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

So the nonInlineNaN() optimization was not helpful?

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:53
> +#define NaN std::numeric_limits<double>::quiet_NaN()
> +#define Inf std::numeric_limits<double>::infinity()

Really?

[Ryosuke Niwa]

This patch broke WinCairo build but I suspect that we just need to kick the bot:
http://build.webkit.org/builders/WinCairo%20Debug%20%28Build%29/builds/7170

[Gavin Barraclough]

(In reply to comment #8)
> (From update of attachment 96833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96833&action=review
> 
> So the nonInlineNaN() optimization was not helpful?

I'm not measuring a performance difference now.  I imagine this may be the kind of thing that changed with the introduction of the JIT.  Inlining methods that accessed globals used to have a significant impact on Interpreter::privateExecute.

> > Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:53
> > +#define NaN std::numeric_limits<double>::quiet_NaN()
> > +#define Inf std::numeric_limits<double>::infinity()
> 
> Really?

Copy & paste throughout the file just seemed a bit code bloaty, since it was used a number of times - but I'm happy to expand these out - will do.

G.

[Gavin Barraclough]

(In reply to comment #9)
> This patch broke WinCairo build but I suspect that we just need to kick the bot:
> http://build.webkit.org/builders/WinCairo%20Debug%20%28Build%29/builds/7170

*nod, agreed.  I searched in the JavaScriptCore directory for any cases of nonInlineNaN that I'd missed (one of the symbols its missing), and can't find any, so I expect the Cairo bot is picking up a stale copy of the JavaScriptCore.def file.

[Geoffrey Garen]

> Copy & paste throughout the file just seemed a bit code bloaty, since it was used a number of times - but I'm happy to expand these out - will do.

An inline function with a short name can remove code bloat without the downsides of a macro.