The default bug view has changed. See this FAQ.

TM: make Math.random threadlocal

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Trunk
x86
Mac OS X
Points:
---
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .10+, status1.9.1 .10-fixed)

Details

(Whiteboard: fixed-in-tracemonkey [qa-examined-191] [qa-noaction-191])

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

8 years ago
Created attachment 395241 [details] [diff] [review]
patch
Assignee: general → gal
(Assignee)

Updated

8 years ago
Attachment #395241 - Flags: review?(shaver)
(Assignee)

Comment 2

8 years ago
  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
(Assignee)

Comment 3

8 years ago
Created attachment 395243 [details] [diff] [review]
patch
Attachment #395241 - Attachment is obsolete: true
Attachment #395241 - Flags: review?(shaver)
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
Attachment #395243 - Flags: review+
(Assignee)

Comment 5

8 years ago
Created attachment 395247 [details] [diff] [review]
 missing upcast to int64 before the << 27
Attachment #395243 - Attachment is obsolete: true
Attachment #395247 - Flags: review?(shaver)
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...
Attachment #395247 - Flags: review?(shaver) → review+
(Assignee)

Comment 7

8 years ago
Created attachment 395250 [details] [diff] [review]
cleanup
Attachment #395247 - Attachment is obsolete: true
(Assignee)

Comment 8

8 years ago
Created attachment 395251 [details] [diff] [review]
more cleanup
Attachment #395250 - Attachment is obsolete: true
Attachment #395251 - Flags: review?(shaver)
(Assignee)

Comment 9

8 years ago
Created attachment 395252 [details] [diff] [review]
patch
Attachment #395251 - Attachment is obsolete: true
Attachment #395252 - Flags: review?(shaver)
Attachment #395251 - Flags: review?(shaver)
(Assignee)

Comment 10

8 years ago
Comment on attachment 395252 [details] [diff] [review]
patch

Waldo, since this is crypto-sensitive, would you mind taking a look too?
Attachment #395252 - Flags: review?(jwalden+bmo)
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...
Attachment #395252 - Flags: review?(jwalden+bmo) → review+
(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...
(Assignee)

Comment 13

8 years ago
http://hg.mozilla.org/tracemonkey/rev/0307226048c8
Whiteboard: fixed-in-tracemonkey

Comment 14

8 years ago
http://hg.mozilla.org/mozilla-central/rev/0307226048c8
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 15

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b2cb8acf4dfc
status1.9.2: --- → beta1-fixed
Flags: wanted1.9.2+
Blocks: 475585
Comment on attachment 395252 [details] [diff] [review]
patch

Need this on 1.9.1 in order to land bug 475585 there.
Attachment #395252 - Flags: approval1.9.1.10?

Comment 17

7 years ago
Comment on attachment 395252 [details] [diff] [review]
patch

a=LegNeato for 1.9.1.10
Attachment #395252 - Flags: approval1.9.1.10? → approval1.9.1.10+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f3a5a5da42f3
status1.9.1: --- → .10-fixed

Updated

7 years ago
blocking1.9.1: --- → .10+
status1.9.1: .10-fixed → ---
status1.9.1: --- → .10-fixed
Is there a repro case at all for this for testing? It doesn't look like it.
I don't know yet if third party software use these APIs, but this patch breaks ABI.
(Assignee)

Comment 21

7 years ago
Which ABI?
libmozjs.so's ABI
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)
Good news, none of the third party software using libmozjs in debian use these functions.
(Assignee)

Comment 25

7 years ago
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).
js_LeaveTrace *is* used in xpconnect, liveconnect, and nsJSEnvironment. Anyways, nothing else uses it, so it is okay.
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.
(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 ?
(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
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.
> 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.
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey [qa-examined-191] [qa-noaction-191]
Attachment #395252 - Flags: review?(shaver)
You need to log in before you can comment on or make changes to this bug.