Bug 55343 – [ES5] Global Math object should be configurable but isn't

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark S. Miller]

They are both still writable as they should be, so this isn't a big deal -- hence the classification as trivial. If you close this out as a wont-fix, please tag this with DeliberateSpecViolation (a new v8 tag introduced for this purpose) or some such so we can track these.

[Gavin Barraclough]

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

[Gavin Barraclough]

Created attachment 120897 [details]
Fix for JSON object

Our current handling of Math & JSON (treating them as non-configurable vars) allows slightly faster access.  This is probably not significant for typical JSON loads, since in real world usage the overhead of one property access from the global object is nothing compared to the cost of loading JSON data over the network, the host call to the parse function, and running the parser over the JSON data.  Fast access to the Math object is more important since it is used for very small, fast intrinsic functions that will be inlined by the JIT, e.g. abs.  We should fix this bug for the Math object too, but there we will need to pay more concern to avoiding any performance penalty – in the case of the JSON object there is no real justification to retain our current incorrect behavior.

[Gavin Barraclough]

Fixed for JSON object in r103921, updating title to reflect this.

[Mark S. Miller]

(In reply to comment #2)
> Created an attachment (id=120897) [details]
> Fix for JSON object
> 
> [...] Math [...] slightly faster access.  [..]  Fast access to the Math object is more important since it is used for very small, fast intrinsic functions that will be inlined by the JIT, e.g. abs.  We should fix this bug for the Math object too, but there we will need to pay more concern to avoiding any performance penalty [...]


I just tested on WebKit Nightly r103874, and as I expected the properties on the Math object are correctly writable and configurable. Thus, you have to worry about cache invalidation for of calls to these Math functions anyway. Given that, is it still more expensive to have assignment to Math also invalidate these same caches?


> Math.abs = function(x){return x;}
> Math.abs(-7)
-7

[Gavin Barraclough]

(In reply to comment #4)
> Given that, is it still more expensive to have assignment to Math also invalidate these same caches?

Hi Mark,

The way our engine works, if we store Math in a regular property on the global object the calling abs will require 3 independent checks:

1) Check the shape of the global object, ensure there is still a 'Math' property at the expected offset.
2) Load the 'Math' property from a fixed offset in the global object's storage.
3) Check the shape of the Math object, ensure there is still a 'abs' property at the expected offset.
2) Load the 'abs' property from a fixed offset in the Math object's storage.
5) Check the abs function is callable (or is the expected value, or check that it is the abs intrinsic, depending on optimizations).

If 'Math' is a var on the global object instead of a property, and as such non-configurable, we can skip step 1 (Math will always be present at the same offset into the var storage).

We should be able to better optimize the process of access to variables like this – of course the fact we have incorrectly been treating 'Math' as a var has been artificially taking away a key incentive to do so!

cheers,
G.

[Eric Seidel (no email)]

Attachment 120897 [details] was posted by a committer and has review+, assigning to Gavin Barraclough for commit.

[Gavin Barraclough]

Comment on attachment 120897 [details]
Fix for JSON object

Clearing review flag, this was landed in r103921.

[Gavin Barraclough]

Created attachment 165153 [details]
Fix Math object.

This no longer has any bearing on performance.

[Eric Seidel (no email)]

Comment on attachment 165153 [details]
Fix Math object.

I'm surprised we don't have a more centralized test for what global objects are deletable or not. :)  LGTM too.

[Gavin Barraclough]

Fixed in r129241

[Gavin Barraclough]

(In reply to comment #9)
> (From update of attachment 165153 [details])
> I'm surprised we don't have a more centralized test for what global objects are deletable or not. :)  LGTM too.

Funnily enough, this isn't covered by the Test262 suite.  Ideally I think a test should be added there and then we should find a way to start running that, though I think if I import the full Test262 suite into LayoutTests people are going to not like me for the runtime. :-)