Math.log2 should be exact for exact powers #323

Yaffle · 2015-02-26T12:41:42Z

Math.log2(1.0 * (1 << 29)) === 29.000000000000004 in Firefox

cscott · 2015-02-26T22:03:58Z

Really? Does the spec say this should be exact?

ljharb · 2015-02-27T02:44:45Z

es6-shim.js

@@ -1594,6 +1604,10 @@
    Math.imul = MathShims.imul;
  }

+  if (Math.log2(536870912.0) !== 29) {
+    Math.log2 = MathShims.log2;


This would need to use defineProperty to preserve non-enumerability.

@ljharb , assignment preservers enumerability (previously I thought it sets all flags to "true", but it preservers it, seems)

huh, indeed it does, disregard my comment then :-) (although, if there were an engine where it was enumerable, this would be an opportunity to correct that)

ljharb · 2015-02-27T02:46:04Z

I'm all for making Math shims more accurate, even if the spec doesn't require it.

However, we'd need the following:

tests that ensure the behavior is correct
example engines that implement this correctly
a link to the relevant spec section that, at the least, leaves it to "implementation-dependent"

cscott · 2015-04-07T16:15:51Z

Well, at least two projects that I know of have recently switched to core-js for their shims because es6-shim was "too slow". Adding extra cruft to methods to obtain accuracy not required by the spec will not help es6-shim if it slows down the library for all users.

Yaffle · 2015-04-07T16:17:30Z

@ljharb , I have updated my patch a little

Yaffle · 2015-04-07T16:19:09Z

es6-shim.js

+      if (value === 0) {
+        return -1 / 0;
+      }
+      var e = Math.min(Math.floor(Math.log(value) / Math.LN2), 1023);


instead of 1023 there should be Math.floor(Math.log(Number.MAX_VALUE) / Math.LN2) - 1 constant or something (?)

if that's where 1023 comes from, then absolutely a named constant would be ideal.

I need to think how to change this.
Seems, this is a little wrong way of integer part calculation...

ljharb · 2015-04-07T17:49:11Z

@cscott What is "too slow"? Which methods? Did they use profiling to determine that?

ljharb · 2015-04-07T17:51:24Z

es6-shim.js

@@ -1498,7 +1498,11 @@
    },

    log2: function (value) {
-      return Math.log(value) * Math.LOG2E;
+      if (value === 0) {
+        return -1 / 0;


Can this just be -Infinity? It's a keyword, so there's no advantage to the division by zero.

if you want

zloirock · 2015-04-07T18:12:05Z

@ljharb

What is "too slow"? Which methods? Did they use profiling to determine that?

Simple example, in all modern environment with fully complying with the specification Map and Set, es6-shim replaces it to slow shim with O(n) search complexity for objects. Try with es6-shim and without:

var array = [];
for(var i = 0; i < 100000; i++)array.push([{}, {}]);
array = array.concat(array);
console.time('Map test');
var map = new Map(array);
console.log('Map size: ' + map.size);
console.timeEnd('Map test');

BTW, in the specification clearly states:

Map object must be implemented using either hash tables or other mechanisms that, on average, provide access times that are sublinear on the number of elements in the collection.

ljharb · 2015-04-07T18:35:44Z

@zloirock they're not actually fully complying if the shim is replacing them (zero engines fully comply at the moment). But yes, that is a limitation, since it's impossible to a) not mutate the objects and also b) have O(1) lookup for them.

However, I've filed #326 so that in this situation, we use the native Map for O(1) lookup, which should alleviate the speed issue.

ljharb · 2015-04-07T18:36:18Z

@Yaffle This code is totally fine now, thanks! Could you also add a test that fails without your changes?

zloirock · 2015-04-07T18:39:13Z

@ljharb

they're not actually fully complying if the shim is replacing them

they're fully complying, but your subclassing detection is broken :)

that is a limitation, since it's impossible

but why core-js and some other shims polyfill it? :D

not mutate the objects

in the spec I don't see anything about it.

ljharb · 2015-04-07T18:45:08Z

@zloirock core-js must be modifying the objects. I'd be happy to accept a pull request if you know of a way to do it, or a link to their implementation. Nothing must ever mutate unless the spec requires it to, so if it's mutating, it's violating the spec.

As for the subclassing detection, please file an issue or open a PR, but I don't think it's broken - you can't actually subclass the native Map/Set implementations at the moment precisely because of how the test fails without the shim ¯_(ツ)_/¯

Yaffle · 2015-04-07T18:46:13Z

@zloirock object mutation is not good and may even affect performance (I hope yo read this page - https://developers.google.com/v8/design )
Anyway, this depends on the use case: if some user wants to store objects in Map AND thinks that it is OK to mutate them, then He can use core-js.

If the number of object is not big or he can map.set(object.uid) or he wants to store only numbers/strings, he may use es6-shim.

zloirock · 2015-04-07T18:47:41Z

@Yaffle broken and slow collections much worse :) Currently, it's required only for old IE, other actual browsers has native collections. Better fix broken methods than replace all collection :)

ljharb · 2015-04-07T18:48:59Z

Let's debate this on another issue, this isn't the one for it, but no, slow is infinitely better than broken (ours is not broken in any way), and mutation is absolutely broken (and as pointed out, insanely deoptimizes objects in v8, so it'd end up being slower in many ways as well).

zloirock · 2015-04-07T18:52:51Z

@ljharb this line should not work in ES6. All modern browsers, includes V8, contains native collections with correct logic. All from me :)

zloirock · 2015-04-07T18:56:57Z

Oh, it's more: babel/babel#1172

ljharb · 2015-04-07T18:58:40Z

@zloirock How then can you subclass Map/Set without class? I'll confirm with TC39 but I'm reasonably sure that's, at best, an unintended side effect.

zloirock · 2015-04-07T19:01:38Z

@ljharb see link. It's only emulation, but correct in most case and much better replacing correct collection to broken.

Yaffle · 2016-01-05T06:06:29Z

@ljharb , example engines that implement this correctly: the engine of Edge, Chrome, Safari, Firefox (with an exception of Math.log2 results under Windows), Konqueror

Math.log2 should be exact for exact powers

22f07d4

Math.log2(1.0 * (1 << 29)) === 29.000000000000004 in Firefox

Yaffle closed this Feb 26, 2015

ljharb reviewed Feb 27, 2015
View reviewed changes

Yaffle reopened this Apr 7, 2015

Yaffle added 2 commits April 7, 2015 21:12

Update es6-shim.js

be48417

Update es6-shim.js

e7ff7c8

Yaffle reviewed Apr 7, 2015
View reviewed changes

ljharb reviewed Apr 7, 2015
View reviewed changes

-Infinity

3d803cf

jaawerth mentioned this pull request Apr 7, 2015

broken subclassing of Map and Set babel/babel#1188

Closed

Yaffle added 2 commits April 8, 2015 08:55

Update es6-shim.js

6ce16c9

Update es6-shim.js

633ab14

Yaffle closed this Apr 8, 2015

ljharb added the math accuracy label Apr 13, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Math.log2 should be exact for exact powers #323

Math.log2 should be exact for exact powers #323

Yaffle commented Feb 26, 2015

cscott commented Feb 26, 2015

ljharb Feb 27, 2015

Yaffle Apr 7, 2015

ljharb Apr 7, 2015

ljharb commented Feb 27, 2015

cscott commented Apr 7, 2015

Yaffle commented Apr 7, 2015

Yaffle Apr 7, 2015

ljharb Apr 7, 2015

Yaffle Apr 7, 2015

ljharb commented Apr 7, 2015

ljharb Apr 7, 2015

Yaffle Apr 7, 2015

zloirock commented Apr 7, 2015

ljharb commented Apr 7, 2015

ljharb commented Apr 7, 2015

zloirock commented Apr 7, 2015

ljharb commented Apr 7, 2015

Yaffle commented Apr 7, 2015

zloirock commented Apr 7, 2015

ljharb commented Apr 7, 2015

zloirock commented Apr 7, 2015

zloirock commented Apr 7, 2015

ljharb commented Apr 7, 2015

zloirock commented Apr 7, 2015

Yaffle commented Jan 5, 2016

Math.log2 should be exact for exact powers #323

Math.log2 should be exact for exact powers #323

Conversation

Yaffle commented Feb 26, 2015

cscott commented Feb 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Feb 27, 2015

cscott commented Apr 7, 2015

Yaffle commented Apr 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Apr 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zloirock commented Apr 7, 2015

ljharb commented Apr 7, 2015

ljharb commented Apr 7, 2015

zloirock commented Apr 7, 2015

ljharb commented Apr 7, 2015

Yaffle commented Apr 7, 2015

zloirock commented Apr 7, 2015

ljharb commented Apr 7, 2015

zloirock commented Apr 7, 2015

zloirock commented Apr 7, 2015

ljharb commented Apr 7, 2015

zloirock commented Apr 7, 2015

Yaffle commented Jan 5, 2016