Details
- Reviewers
• leper elexis Itms - Commits
- rP19367: Revert 64bit number conversions added in r19189, don't add the previous long…
- Trac Tickets
- #3848
Test whether compilation works on 32 and 64-bit Linux, and on Windows.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
IRC discussion:
20:56 < echotangoecho> elexis: there is a problem with D190 20:56 < WildfireRobot> D190: Microsecond-precise JS performance measurement [Needs Review] – <https://code.wildfiregames.com/D190> 20:56 < echotangoecho> we can only send i32's to js 20:56 < elexis> what is it? 20:57 < echotangoecho> the value we get in js often wraps 20:57 < echotangoecho> well ok 20:57 < echotangoecho> it is not that bad 20:57 < echotangoecho> but I get a negative value in JS now 20:57 < elexis> huh? 20:57 < elexis> thought you added i64 20:57 < echotangoecho> yes, but it casts to i32 20:57 < elexis> wth 20:58 < echotangoecho> as there is some limitation that we can only use i64 (I think) 20:58 < echotangoecho> i32* 20:58 < elexis> u64 then as that function shouldnt return negatives 20:58 < echotangoecho> yeah, JS doesn't support 64 bit values :/ 21:01 < echotangoecho> actually we probably shouldn't even have the functions for sending i64/u64 to js, as those are useless 21:02 < elexis> std::string? 21:02 < Philip> JS numbers are doubles, which only have roughly 53 bits of precision, so they can't represent all i64s 21:02 < echotangoecho> hmm 21:02 < Philip> If you know your numbers are integers <2^53 then converting to double should be okay though 21:03 < elexis> ENSURE? 21:03 < echotangoecho> but at that point, it doesn't really make sense to provide explicit i64/u64 scriptconversions, we should cast to double where the value is sent 21:03 < elexis> yes 21:04 < echotangoecho> so we should revert the patch I did
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/491/ for more details.
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/492/ for more details.
Just removing the functions might not work, current MacOS compilations fails in replay.cpp line 57:
m_ScriptInterface.SetProperty(attribs, "timestamp", std::time(nullptr));
presumably std::time is a long behind the scenes? Perhaps we could just cast that to a double or something.
edit: got the idea from this very patch in fact, so never mind this does work, sry :P
As mentioned by Philip in that log:
21:02 < Philip> JS numbers are doubles, which only have roughly 53 bits of precision, so they can't represent all i64s
21:02 < Philip> If you know your numbers are integers <2^53 then converting to double should be okay though
From http://ecma262-5.com/ELS5_HTML.htm#Section_8.5we can confirm this statement:
Note that all the positive and negative integers whose magnitude is no greater than 253 are representable in the Number type (indeed, the integer 0 has two representations, +0 and −0).
Therefore pretending to be able to convert (all) 64bit ints to JS numbers might be bad indeed.
We should carefully examine whether converting C++ doubles and long doubles to JS numbers doesn't have the same boundary issues.
I'm accepting this, but I'm not 100% happy with the code situation.
Double/float aren't a well defined types either and it's implementation depends on the platform and compiler used.
For that reason it isn't allowed to be used in the simulation! (Search for double in the simulation2 directory or use leper 2016* | grep "float" on the #0ad-dev logs to confirm.)
Example:
At http://en.cppreference.com/w/cpp/types/numeric_limits/max we can see the min/max values constant names of the types.
For double it is DBL_MAX.
For my OS it is defined at /usr/lib/gcc/x86_64-linux-gnu/5/include/float.h:
`#define DBL_MAX __DBL_MAX__
The value of __DBL_MAX__ can be found using gcc -E -dM -x c <(true) and for me it yields:
#define __DBL_MAX__ ((double)1.79769313486231570815e+308L)
It seems it works in many cases and it is already used in SavedGame.cpp to save the timestamp.
If we fix it, it should be fixed in all occurances, not only Replay.cpp. This should be done in a separate patch.
It was discussed tody in #0ad-dev and Philip gave the advice to implement something like UnsafeFloatToJSVal and UnsafeU64ToJSVal if we want to keep those conversions.
Converting from 64bit to 53bit JS Number doesn't work, but the other way around looks like it could be kept.
Confusing yes, but it could also be addressed by writing comments informing people that not every 64bit number can be represented by a JS number.
Double and floats are also not safe for simulation, so not having a comment about this seems just as wrong.
Then again we have tests in test_ScriptConversions.cpp for the limits and the double limit actually matches the JS number limit.
Therefore using a double conversion actually seems more honest than using a 64bit conversion (and pretending that would work for all 64bit numbers).
Because the previous long and unsigned long conversions were already bugging rP17941, I actually prefer keeping those removed over adding them back and adding a comment warning people to not use them.
Double/float aren't a well defined types either and it's implementation depends on the platform and compiler used.
IEEE 754 floating point numbers of single and double precision are quite well defined. There is just some difference in what a given calculation can return, which might differ based on platform, compiler, FPU rounding modes, compiler flags, or CPU extensions using higher precision internally. All of which result in valid results, but not in something we can (or should) rely on to be equal across all supported platforms and configurations.
It was discussed tody in #0ad-dev and Philip gave the advice to implement something like UnsafeFloatToJSVal and UnsafeU64ToJSVal if we want to keep those conversions.
Good suggestion, that might be useful fo the floating point conversions, and then possibly adding a linting rule that fails hard if those occurs somewhere close to the simulation.
Converting from 64bit to 53bit JS Number doesn't work, but the other way around looks like it could be kept.
JS numbers are 64 bit wide (most NaN values are used for storing other things, but that is something that is left to the implementation).[1]
The 53-bit number comes from the size of the fraction part of the IEEE 754 double precision binary representation. (Actually that is 52 wide, but the initial 1 isn't stored.) This restricts the range of integers where for every n n+1 can also be stored (take some care around the edges, or read the ECMAScript standard which explains what it considers "safe integers" [2]).
Then again we have tests in test_ScriptConversions.cpp for the limits and the double limit actually matches the JS number limit.
Which it needs to, else the JS engine would violate the ECMAScript standard.
[1] https://www.ecma-international.org/ecma-262/6.0/#sec-terms-and-definitions-number-value
[2] https://www.ecma-international.org/ecma-262/6.0/#sec-number.issafeinteger
(Perhaps something with the JS type UInt64 would be possible, but then we may not use the Number type anymore in any variable that contains one of these numbers as a factor or summand and also for numbers that use these numbers. So it would have to be used everywere, but then we can't use floats anymore in the same place)