105476 - Add ASM version of js_CompareAndSwap for OS/2

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Kaply [:mkaply]]

Our performance team has determined that js_CompareAndSwap 
calling PR_Lock and PR_Unlock is a performance issue.

I am adding an ASM version for OS/2.

Note OS/2 does not have inline assembly, so I have to add 
an ASM file. It is in the diff.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Add OS/2 ASM implementation of js_CompareAndSwap

Comment 2

[Phil Schwartau]

cc'ing reviewers for this patch -

Comment 3

[timeless]

i'd imagine the file should be MPL not NPL, but i could be wrong. Am i?

Comment 4

[Mike Kaply [:mkaply]]

I couldn't decide.

The ASM in the file is derivative of the Windows ASM in jslock.c which is an NPL 
file.

Does that matter?

Comment 5

[Gervase Markham [:gerv]]

If it's a derivative of an NPL file, it should be tri-licensed, as the NPL file
should also be tri-licensed - we just haven't yet got a script smart enough to
do it automatically for us ;-)

The license policy states that new files which are derivatives of any code even
partly licensed under the NPL should be triple-licensed.

Gerv

Comment 6

[Mike Shaver (:shaver emeritus)]

What does your confusing phrase "even partly licensed under the NPL" mean, exactly?

Comment 7

[Gervase Markham [:gerv]]

I mean NPL, NPL/GPL, NPL/LGPL, NPL/MPL, NPL/LGPL/GPL, or any other combination
under the sun which includes the NPL.

Gerv

Comment 8

[Mike Kaply [:mkaply]]

I guess I'll put it under the MPL then because we don't have a process for the 
other.

Comment 9

[Gervase Markham [:gerv]]

mkaply, it's not legal to put it under the MPL. If it's derived from an NPLed
file, you can't just relicense it. I know this sucks, but it's true. You can
either license it under the NPL, in which case when we relicense files of that
type it'll get tri-licensed, or you can tri-license it to start with and save us
the bother :-)

Gerv

Comment 10

[timeless]

Do we have a way of indicating where original code came from? for new files we
have Original Code: <authorname> or Contributors: <authorname> (Original Code),
and Orginal Code is ..., perhaps it would be nice to have Original Code:
<cvspathname>?

... sorry to cause so much spam over 2 letters.

Comment 11

[Gervase Markham [:gerv]]

No, we don't. You may, however, wish to copy the Contributors: section, if there
is one, out of the old file.

Gerv

Comment 12

[Mike Kaply [:mkaply]]

Attachment: License is now MPL (I think the file is different enough) also, only do this for VACPP

Comment 13

[Gervase Markham [:gerv]]

mkaply: how can I make this more clear? :-)

You have two choices. 

- If you believe the code contains none of the original NPL-ed file, you must
license the code under the MPL tri-license, in accordance with the mozilla.org
licensing policy for completely new files.
http://www.mozilla.org/MPL/license-policy.html

- If you believe the code contains some of the original NPL-ed file, the
derivative work may be either NPL-ed, in which case we will NPL tri-license it
later, or you can skip straight to using the NPL tri-license.

Gerv

Comment 14

[Mike Kaply [:mkaply]]

OK now you have managed to utterly confuse me.

Why are there files in the tree that are MPL/GPL and not MPL/GPL/LGPL?

Comment 15

[Mike Kaply [:mkaply]]

Attachment: I get it now, I really do - triple license ASM file plus diff

Comment 16

[Gervase Markham [:gerv]]

> Why are there files in the tree that are MPL/GPL and not MPL/GPL/LGPL?

Because they were originally licensed that way, and we have not yet obtained the
copyright holders' permission to change them :-)

On that note, expect a mass email from me sometime tonight (probably) about
that. :-)

Gerv

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 55822 [details] [diff] [review]
I get it now, I really do - triple license ASM file plus diff

Thanks, only one really picky nit:

 !( (defined(_WIN32) && defined(_M_IX86)) ||                               \
        (defined(__GNUC__) && defined(__i386__)) ||                            \
        (defined(SOLARIS) && defined(sparc) && defined(ULTRA_SPARC)) ||        \
-       defined(AIX) )
+       defined(AIX) || defined(XP_OS2_VACPP))


See the extra space after '(AIX)' in 'defined(AIX) )' in the minus line?

Restore that to match the space after the opening paren of the ! operand,
and sr=[e-mail].

/be

Comment 18

[Brendan Eich [:brendan]]

mkaply: please confirm that, with the fix to bug 105571 (checked in on 10/19),
you don't see very many calls to js_CompareAndSwap.  If you do, I'm interested
in the stack backtraces, and the classes of JS objects (or what code created them).

/be

Comment 19

[Sam Emrick]

Brendan, I agree, most of the compareandswap come from js_GetSlotThreadSafe, so 
when we get up to 0.96 then this bit of asm be less needed. On the other hand, 
I think secondary benefit is it puts os/2 on the more common #ifndef NSPR_LOCK 
code path through the jslock code.
Sam

Comment 20

[Mike Kaply [:mkaply]]

Fix checked in.

I'll have our performance team run with a current build and see if they see as 
much js_CompareAndSwap in the traces as they used to before 105571

Comment 21

[Phil Schwartau]

Has the performance analysis been successful? I don't have OS/2
to verify this bug; thanks - 

Comment 22

[Phil Schwartau]

Is this OK now on 0S/2? If I don't hear otherwise, I'm going to
mark this one Verified soon...

Comment 23

[Mike Kaply [:mkaply]]

We're good. I don't have numbers yet though.