772722 - Remove superfluous usage of Atomics in SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

With a single-threaded runtime these are mostly unhelpful.  The others are just as well served by volatile access.

https://tbpl.mozilla.org/?tree=Try&rev=adbea35924d3

Comment 1

[Terrence Cole [:terrence]]

One of the uses of Atomics is for a fast path in PRMJ_Now() on windows.  In windows, getting a high-quality (1us precision) time is a PITA and takes an eternity: 2 spinlocks, several syscalls and a fair bit of logic + branches.  It is easy, however, to get a low-quality time with ~15ms granularity.  So, to avoid making startup slow in the browser, we make the first 10 calls to PRMJ_Now() use the low-resolution timer.  The number 10 is the number of PRMJ_Now() calls that the browser made from a cold start to a blank home page, as of 5 years ago this Friday.

On the machine I tested, the high-quality timer takes about 290ns to get a time and the low-quality timer takes ~0ns.  Thus, we are saving: ~3us.

And since PRMJ_Now() could potentially be called from multiple threads at some point during the operation of firefox, naturally we have to use atomic ops to protect the sanctity of these 10 calls.

Comment 2

[Terrence Cole [:terrence]]

Attachment: v0: Kill atomics.

Updated to also kill off the premature optimization at the front of PRMJ_Now() on windows.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 641091 [details] [diff] [review]
v0: Kill atomics.

Review of attachment 641091 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfriendapi.h
@@ +166,5 @@
>      /*
>       * If non-zero, we were been asked to call the operation callback as soon
>       * as possible.
>       */
> +    volatile int        interrupt;

Let's keep this explicitly sized since it is at an ABI boundary.

Comment 4

[Terrence Cole [:terrence]]

Ah, good point.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc4e44365c4

Comment 5

[Terrence Cole [:terrence]]

And backed out in
https://hg.mozilla.org/integration/mozilla-inbound/rev/70afb9434e3e
for windows linker bustage.

Comment 6

[Terrence Cole [:terrence]]

Insanity unceasing! There is this hunk in shell/js.cpp's main():

#ifdef XP_WIN
    // Set the timer calibration delay count to 0 so we get high
    // resolution right away, which we need for precise benchmarking.
    extern int CALIBRATION_DELAY_COUNT;
    CALIBRATION_DELAY_COUNT = 0;
#endif

!?!?

Comment 7

[Terrence Cole [:terrence]]

Attachment: v1: bustage fixed.

One more windows try run:
https://tbpl.mozilla.org/?tree=Try&rev=d23090e4061a

Comment 8

[Terrence Cole [:terrence]]

Fox 3!

https://hg.mozilla.org/integration/mozilla-inbound/rev/5eacd4fc78e4

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8d98dea0ba5f - something was causing intermittent but quite frequent mochitest-4 shutdown crashes like https://tbpl.mozilla.org/php/getParsedLog.php?id=13443472&tree=Mozilla-Inbound, and although I tried to pin it on the push before you with a half dozen retriggers on every platform, I didn't get a single one before you landed.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oops. I guess JS_DropPrincipals is also called on the background thread. So that atomic is still necessary.

Comment 11

[Ed Morley [:emorley]]

I believe this may be the cause of a winxp pgo-only jsreftest permanent failure I'm seeing:

https://tbpl.mozilla.org/php/getParsedLog.php?id=13450157&tree=Mozilla-Inbound

{
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma_3/RegExp/15.10.6.2-2.js | Section 6 of test -
regexp = /abc/gi
string = 'AbcaBcabC'
ERROR !!! regexp MATCHED when we expected it to fail !!!
Expect: null
Actual: ["Abc"]
 Type mismatch, expected type object, actual type string Expected value 'null', Actual value '["Abc"]'  item 6
...etc
}

Plausible?

Comment 12

[Ed Morley [:emorley]]

Meant to say: I've triggered some extra pgo runs to try and confirm, but turnaround time is such that we'll hose merging inbound for the rest of the day. Unless this definitely couldn't be the cause, I'd like to back out, retrigger pgo on tip and see from there really...

Comment 13

[Terrence Cole [:terrence]]

(In reply to Ed Morley [:edmorley] from comment #12)
> Meant to say: I've triggered some extra pgo runs to try and confirm, but
> turnaround time is such that we'll hose merging inbound for the rest of the
> day. Unless this definitely couldn't be the cause, I'd like to back out,
> retrigger pgo on tip and see from there really...

Bill has the right of it.  The atomic inc/dec on the principals refcount is almost certainly to blame.

I'll do some benchmarking today to see how hot this path is.  If it is cold, we can just wrap a mutex on platforms that don't have OS or compiler support for atomics.  Otherwise, we'll have to break out the asm, and that's not really the route we want to go.

Comment 14

[Ed Morley [:emorley]]

Sorry, I completely forgot to update this bug. After more retriggers, it was found bug 772303 was to blame - and a backout of that turned the test green again.

Sorry for the false alarm! :-)

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Can this land?

Comment 16

[Terrence Cole [:terrence]]

Thanks for the reminder; I'll rebase and see if it is still green on try.

Comment 17

[Terrence Cole [:terrence]]

We don't actually want to take this.  I learned this week that our testing on ARM is frequently just _not_.  Since ARM has a weaker memory model than x86, these sites will break horribly if we don't keep the atomic ops on them.  Since we need CondVar's for WinXP anyway, this also doesn't get us substantially closer to removing NSPR.  Hopefully someone will add real threading primitives to MFBT or SM soon, at which point we can worry more seriously about transitioning off of NSPR.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Most of these atomics are unnecessary. The one on propertyRemovals seems likely to be slowing us down, if only a little bit. Why don't we just take out all the ones except on the principals? It would be a nice code cleanup.

Comment 19

[Terrence Cole [:terrence]]

Good point!  I'll go ahead and do that.

Comment 20

[Terrence Cole [:terrence]]

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=102ede70876b

Re-pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/475ae0fac54e

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/475ae0fac54e