WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
16777
eliminate JSC::NaN and JSC::Inf
https://bugs.webkit.org/show_bug.cgi?id=16777
Summary
eliminate JSC::NaN and JSC::Inf
Darin Adler
Reported
2008-01-07 14:12:53 PST
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.
Attachments
patch -- I'd land it if SunSpider didn't seem to slow down by 0.5%
(19.66 KB, patch)
2008-01-07 17:08 PST
,
Darin Adler
no flags
Details
Formatted Diff
Diff
The patch (no perf impact)
(24.69 KB, patch)
2011-06-10 18:27 PDT
,
Gavin Barraclough
sam
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Darin Adler
Comment 1
2008-01-07 17:08:15 PST
Created
attachment 18321
[details]
patch -- I'd land it if SunSpider didn't seem to slow down by 0.5%
David Kilzer (:ddkilzer)
Comment 2
2008-01-08 03:54:09 PST
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
Comment 3
2008-01-08 09:51:28 PST
(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
Comment 4
2011-06-10 18:25:28 PDT
***
Bug 19272
has been marked as a duplicate of this bug. ***
Gavin Barraclough
Comment 5
2011-06-10 18:25:55 PDT
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
Comment 6
2011-06-10 18:27:56 PDT
Created
attachment 96833
[details]
The patch (no perf impact)
Gavin Barraclough
Comment 7
2011-06-10 19:03:16 PDT
Fixed in
r88587
Darin Adler
Comment 8
2011-06-10 22:14:02 PDT
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
Comment 9
2011-06-10 23:11:43 PDT
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
Comment 10
2011-06-10 23:44:50 PDT
(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
Comment 11
2011-06-10 23:46:20 PDT
(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
Comment 12
2011-06-13 16:23:04 PDT
> 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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug