Closed Bug 511328 Opened 12 years ago Closed 12 years ago
TM: make Math
No description provided.
Assignee: general → 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 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+
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+
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...
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 395252 [details] [diff] [review] patch Need this on 1.9.1 in order to land bug 475585 there.
Attachment #395252 - Flags: approval22.214.171.124?
Comment on attachment 395252 [details] [diff] [review] patch a=LegNeato for 126.96.36.199
Attachment #395252 - Flags: approval188.8.131.52? → approval184.108.40.206+
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.
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.
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]
You need to log in before you can comment on or make changes to this bug.