511328 - TM: make Math.random threadlocal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

  string:              1.009x as fast    324.9ms +/- 0.2%   322.0ms +/- 0.2%     significant
    base64:            1.039x as fast     20.9ms +/- 0.6%    20.1ms +/- 0.6%     significant
    fasta:             *1.023x as slow*   67.8ms +/- 0.2%    69.3ms +/- 0.2%     significant
    tagcloud:          1.008x as fast    100.2ms +/- 0.2%    99.5ms +/- 0.3%     significant
    unpack-code:       ??                 98.9ms +/- 0.4%    99.4ms +/- 0.2%     not conclusive: might be *1.005x as slow*
    validate-input:    1.098x as fast     37.1ms +/- 0.6%    33.8ms +/- 0.8%     significant

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 395243 [details] [diff] [review]
patch

I sure won't miss that JSLL_* stuff, and it looks like a faithful translation.

r=shaver

Comment 5

[Andreas Gal :gal]

Attachment: missing upcast to int64 before the << 27

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 395247 [details] [diff] [review]
 missing upcast to int64 before the << 27

Better! Can we just make random_next return uint64, though?  I'm not the guy to do the analysis of those effects, to be sure...

Comment 7

[Andreas Gal :gal]

Attachment: cleanup

Comment 8

[Andreas Gal :gal]

Attachment: more cleanup

Comment 9

[Andreas Gal :gal]

Attachment: patch

Comment 10

[Andreas Gal :gal]

Comment on attachment 395252 [details] [diff] [review]
patch

Waldo, since this is crypto-sensitive, would you mind taking a look too?

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 395252 [details] [diff] [review]
patch

Throughout: use the u?int64_t typenames rather than u?int64.

>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h

>+    /* Random number generator state, used by jsmath.c. */

cpp


>diff --git a/js/src/jsmath.cpp b/js/src/jsmath.cpp

I do so love these random_* methods, it's like we have to poor-man's namespace because we can't have a |class RNG|!  Doesn't need to be fixed now, maybe a followup...

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #10)
> Waldo, since this is crypto-sensitive, would you mind taking a look too?

More than happy to take a look, but this seems a bit of a non sequitur...

Comment 13

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/0307226048c8

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0307226048c8

Comment 15

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b2cb8acf4dfc

Comment 16

[Reed Loden [:reed]]

Comment on attachment 395252 [details] [diff] [review]
patch

Need this on 1.9.1 in order to land bug 475585 there.

Comment 17

[LegNeato]

Comment on attachment 395252 [details] [diff] [review]
patch

a=LegNeato for 1.9.1.10

Comment 18

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f3a5a5da42f3

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Is there a repro case at all for this for testing? It doesn't look like it.

Comment 20

[Mike Hommey [:glandium]]

I don't know yet if third party software use these APIs, but this patch breaks ABI.

Comment 21

[Andreas Gal :gal]

Which ABI?

Comment 22

[Mike Hommey [:glandium]]

libmozjs.so's ABI

Comment 23

[Mike Hommey [:glandium]]

To be more precise, this breaks js_LeaveTrace() and js_CanLeaveTrace(). I'm currently going through the third party software using libmozjs in debian to see if any are using these functions apart from mozilla code itself (liveconnect and xpconnect do)

Comment 24

[Mike Hommey [:glandium]]

Good news, none of the third party software using libmozjs in debian use these functions.

Comment 25

[Andreas Gal :gal]

We didn't really intend those functions to be part of the ABI. In particular all of jscntxt.h is not part of the ABI. Its not even part of the friends ABI we use internally in xpconnect. If you need those to functions, we should talk about that and add a proper public API (I suspect you shouldn't need them though, whatever you do we should make sure it either works on trace, or we fall of trace inside the engine before embedding code executes).

Comment 26

[Mike Hommey [:glandium]]

js_LeaveTrace *is* used in xpconnect, liveconnect, and nsJSEnvironment. Anyways, nothing else uses it, so it is okay.

Comment 27

[Jeff Walden [:Waldo]]

Those are all incestuous and not good style, diving under the covers, so to speak.  They use this only for speed reasons, and we'd like to fix them not to do so at some point.  But ultimately, we have more important things to worry about than absolute API purity.  JS and the browser are intertwined, it's a fact of life not worth getting incredibly upset about.

Comment 28

[Mike Hommey [:glandium]]

(In reply to comment #27)
> JS and the browser are intertwined, it's a
> fact of life not worth getting incredibly upset about.

"Incredibly upset" ? Haven't you read comment 26 and comment 24 ?

Comment 29

[Brendan Eich [:brendan]]

(In reply to comment #28)
> (In reply to comment #27)
> > JS and the browser are intertwined, it's a
> > fact of life not worth getting incredibly upset about.
> 
> "Incredibly upset" ? Haven't you read comment 26 and comment 24 ?

Waldo certainly exaggerated. Indeed he may be more upset (I was) to hear of ABI including js_LeaveTrace.

Anyway, with old C code one tends to find growth of the "circle of trust" in terms of Mozilla modules growing over time. We will prune back. But in no case are these js_* functions for general consumption.

/be

Comment 30

[Jeff Walden [:Waldo]]

I have.  My interpretation of comment 26 was that it was implying those were evil witches in gingerbread houses, likely to lead little children astray, and thus something worth prioritizing for fixing.  :-)  I was simply saying it's a necessary risk for now, and that it's a fairly small one not deserving of too much concern in any case.

Comment 31

[Mike Hommey [:glandium]]

> But in no case are these js_* functions for general consumption. 

But the headers as they are shipped at the moment don't help very much, especially because most mix things that are exported and things that aren't, and also because most aren't supposed to be public at all. In other words, I'd very much like more feedback on bug 554088.

Enough from me in this bug. Sorry for the noise.