Closed Bug 772722 Opened 8 years ago Closed 8 years ago

Remove superfluous usage of Atomics in SpiderMonkey

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: terrence, Assigned: terrence)

Details

(Whiteboard: [js:t])

Attachments

(1 file, 1 obsolete file)

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
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.
Attached patch v0: Kill atomics. (obsolete) — Splinter Review
Updated to also kill off the premature optimization at the front of PRMJ_Now() on windows.
Attachment #641091 - Flags: review?(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.
Attachment #641091 - Flags: review?(luke) → review+
And backed out in
https://hg.mozilla.org/integration/mozilla-inbound/rev/70afb9434e3e
for windows linker bustage.
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

!?!?
One more windows try run:
https://tbpl.mozilla.org/?tree=Try&rev=d23090e4061a
Attachment #641091 - Attachment is obsolete: true
Attachment #641205 - Flags: review+
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.
Oops. I guess JS_DropPrincipals is also called on the background thread. So that atomic is still necessary.
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?
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...
(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.
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! :-)
Whiteboard: [js:t]
Can this land?
Thanks for the reminder; I'll rebase and see if it is still green on try.
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
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.
Good point!  I'll go ahead and do that.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
https://hg.mozilla.org/mozilla-central/rev/475ae0fac54e
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.