Last Comment Bug 720094 - Only have one implementation of JSDOUBLE_IS_NaN, not two-via-ifdefs
: Only have one implementation of JSDOUBLE_IS_NaN, not two-via-ifdefs
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- trivial (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 720070
  Show dependency treegraph
Reported: 2012-01-21 00:00 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2016-02-19 19:25 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (799 bytes, patch)
2012-01-21 00:00 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Real patch (738 bytes, patch)
2012-01-21 00:08 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Patch that definitely passes try, including PGO builds (847 bytes, patch)
2012-01-22 00:37 PST, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-21 00:00:00 PST
Created attachment 590441 [details] [diff] [review]

JSDOUBLE_IS_NaN contains two separate implementations, one for MIPS, and one for everything else.  I don't see why there can't just be one.  The attached patch implements this.

(Tangentially, all this double-bit-manipulation/frobbing stuff needs to move out of the JS engine.  The same functionality is also coded up in content/ in different form, but there's no good reason to have to implement it all twice.  But that should be another bug.)
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-21 00:04:59 PST
Comment on attachment 590441 [details] [diff] [review]

Review of attachment 590441 [details] [diff] [review]:

Er, scratch that, I was looking at the wrong part of IEEE-754.  The right patch in a sec.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-21 00:08:06 PST
Created attachment 590442 [details] [diff] [review]
Real patch
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-21 02:53:21 PST
Comment on attachment 590442 [details] [diff] [review]
Real patch

glandium pointed me at the trail of tears in bug 640494 and bug 653056 -- namely, that MSVC+PGO miscompiles the MIPS bitwise algorithm here.  So this version's a no-go.  I'll do some experimenting and figure out an approach that'll appease all the compiler gods, then re-request review once I've done that.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-22 00:37:45 PST
Created attachment 590547 [details] [diff] [review]
Patch that definitely passes try, including PGO builds

Okay, this should really do it.  I did a try push that also enabled PGO, and while there seems to be the usual level of orangeness to it, there doesn't seem to be any PGO-specific orange.

And despite that not mentioning PGO explicitly as a listed "Win pgo" item, PGO definitely happened.  Ed Morley says the string to search for in the logs is "linker max virtual size", and that's definitely present in them.  So I think this is good to go.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-23 14:51:13 PST

To be safe, this probably shouldn't be closed until we've had a round of PGO builds on m-c pass with it.
Comment 6 Marco Bonardo [::mak] 2012-01-24 04:55:11 PST

Is not PGO on inbound enough to close this?
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-24 05:24:42 PST
Inbound doesn't do PGO builds, I thought.  But, looking back, I see it does them "sometimes", with no rhyme or reason I can see.  As long as those passed, we should be good.
Comment 8 Marco Bonardo [::mak] 2012-01-24 05:26:18 PST
pgo builds are made every 3 hours, on all trees. Nightlies are also PGO. Merges from inbound to central happen only on pushes that got a green pgo build+tests.
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-25 16:27:54 PST
In the interest of developer sanity at having to think about different implementations of on trunk and branches, it seemed a good idea to get this in branches as well.  I talked it over with akeybl, and he was persuaded to take it in aurora.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-26 15:49:49 PST
...and beta as well:

Note You need to log in before you can comment on or make changes to this bug.