416675 - refactor JSScope locking for reuse on non-native objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Shaver (:shaver emeritus)]

Plan:

- refactor JSScope's locking members into a JSOwnable struct, and adapt the locking macros.  Locking members are ownercx, lock, count, link, file, line

- rewrite the locking macros in terms of ones that operate on the JSOwnable

(- use the new locking macros in arrays for threadsafety!)

Comment 1

[Mike Shaver (:shaver emeritus)]

Attachment: untested, but builds JS_THREADSAFE with NSPR and starts

Comment 2

[Mike Shaver (:shaver emeritus)]

MikeM: would love your abuse of this patch, and subsequent ones, if you get a chance.

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 302430 [details] [diff] [review]
untested, but builds JS_THREADSAFE with NSPR and starts

...and it passes the test suite, so let's see what Mr Bacon thinks of it.

Comment 4

[Mike Shaver (:shaver emeritus)]

Attachment: WIP 2: fix non-JS_THREADSAFE and non-DEBUG builds

Comment 5

[Robert Sayre]

Seems valgrind clean in browser.

Comment 6

[Mike Moening]

So what is the goal of this patch? Speed improvements?  Just curious what the motivation was...

Comment 7

[Mike Shaver (:shaver emeritus)]

The goal is to allow the use of the single-thread-optimized, request-based locking system for objects that don't use JSScope.  One important such set of objects is the new custom-ops Array implementation in bug 322889, which needs threadsafety.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment 302507 [details] [diff] passes the threading tests of bug 404879. No failures debug or opt.

Comment 9

[Mike Shaver (:shaver emeritus)]

Attachment: WIP 3: s/ownable/title/

Better name.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 302954 [details] [diff] [review]
WIP 3: s/ownable/title/

js_TransferTitleLock => js_TransferTitle ;-)
js_DestroyTitle => js_FinishTitle
Comment requirement that JSObjectMap precede JSTitle? FIXME should cite https://bugzilla.mozilla.org/show_bug.cgi?id=408416

Great otherwise!

/be

Comment 11

[Mike Shaver (:shaver emeritus)]

Attachment: updated to review comments

As you requested, so it shall be.

Comment 12

[Mike Shaver (:shaver emeritus)]

Attachment: interdiff for review comments

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 303266 [details] [diff] [review]
updated to review comments

Thanks for the interdiff but please use old on left USENET standard, I'm old too ;-). Also, lose the French spacing in that comment!

/be

Comment 14

[Mike Shaver (:shaver emeritus)]

Dammit, I forgot to fix the French spacing.  I'll bundle it into something else, sorry.

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.409; previous revision: 3.408
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.194; previous revision: 3.193
done
Checking in jslock.c;
/cvsroot/mozilla/js/src/jslock.c,v  <--  jslock.c
new revision: 3.79; previous revision: 3.78
done
Checking in jslock.h;
/cvsroot/mozilla/js/src/jslock.h,v  <--  jslock.h
new revision: 3.39; previous revision: 3.38
done
Checking in jsobj.h;
/cvsroot/mozilla/js/src/jsobj.h,v  <--  jsobj.h
new revision: 3.86; previous revision: 3.85
done
Checking in jsscope.c;
/cvsroot/mozilla/js/src/jsscope.c,v  <--  jsscope.c
new revision: 3.79; previous revision: 3.78
done
Checking in jsscope.h;
/cvsroot/mozilla/js/src/jsscope.h,v  <--  jsscope.h
new revision: 3.56; previous revision: 3.55
done

Comment 15

[Brendan Eich [:brendan]]

Canadian bilingual correctness alert! I'm going to make cbeard say something in bad Frensh to you, just for that :-P.

/be