It sure would be nice to be able to do printf debugging in parallel in selfhosted code.

billm, I r?ing you for this part since it touches js/public

Comment on attachment 760735 [details] [diff] [review] Part 2: Add ParallelSpew intrinsic and Ion inline Review of attachment 760735 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine modulo the comment about the macros. However, it also seems like with `ParallelSpew` (and perhaps some time ago) we're reaching the point that it's worth thinking about a generic way of connecting parallel functions---basically some way to register a "parallel C" function on a function object, so that we don't have to keep inlining things. ::: js/src/builtin/ParallelArray.js @@ +32,5 @@ > + * The ParallelSpew intrinsic is only defined in debug mode, so define a dummy > + * if debug is not on. > + */ > +#ifndef DEBUG > +function ParallelSpew(string) { } Shouldn't this be some sort of macro, so that something like ParallelSpew("a" + "b") doesn't still construct the string "a" + "b" in optimized builds?

(In reply to Niko Matsakis [:nmatsakis] from comment #3) > Comment on attachment 760735 [details] [diff] [review] > Part 2: Add ParallelSpew intrinsic and Ion inline > > Review of attachment 760735 [details] [diff] [review]: > ----------------------------------------------------------------- > > This seems fine modulo the comment about the macros. However, it also seems > like with `ParallelSpew` (and perhaps some time ago) we're reaching the > point that it's worth thinking about a generic way of connecting parallel > functions---basically some way to register a "parallel C" function on a > function object, so that we don't have to keep inlining things. I was thinking of exactly the same thing, to be able to call threadsafe native functions. I'll look at that soonish. > > ::: js/src/builtin/ParallelArray.js > @@ +32,5 @@ > > + * The ParallelSpew intrinsic is only defined in debug mode, so define a dummy > > + * if debug is not on. > > + */ > > +#ifndef DEBUG > > +function ParallelSpew(string) { } > > Shouldn't this be some sort of macro, so that something like > > ParallelSpew("a" + "b") > > doesn't still construct the string "a" + "b" in optimized builds? Good point.

Comment on attachment 760734 [details] [diff] [review] Part 1: Change LossyTwoByteCharsToNewLatin1CharsZ to take ThreadSafeContext. Review of attachment 760734 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/CharacterEncoding.h @@ +12,5 @@ > #include "js/Utility.h" > > #include "jspubtd.h" > > +namespace js { class ThreadSafeContext; } If ThreadSafeContext is in jspubtd.h, then this shouldn't be needed.

Carrying r+, as this revision only removes the inlining, which won't be needed once bug 881988 lands.

Flipping to bhackett since and changes are small and I want to land this now-ish.

Comment on attachment 772104 [details] [diff] [review] Part 1: Change string to chars conversion functions to take ThreadSafeContext. Review of attachment 772104 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/CharacterEncoding.h @@ +141,5 @@ > * will return a NULL chars (which can be tested for with the ! operator). > * This method cannot trigger GC. > */ > extern Latin1CharsZ > +LossyTwoByteCharsToNewLatin1CharsZ(js::ThreadSafeContext *tcx, TwoByteChars tbchars); Would it be ok to just name these tcx parameters as cx? In bug 885758 I use 'cx' for ExclusiveContext parameters, and since all the contexts fulfill the same basic purpose and the type system knows how to distinguish them using different names doesn't seem to add much, except when asJSContext() downcasts are needed. ::: js/src/shell/js.cpp @@ +303,2 @@ > { > + JSLinearString *linear = str->ensureLinear(tcx); Per IRC discussion it is racy to flatten ropes on contending threads, and ensureLinear should take a JSContext. Since this is preexisting though this use can just be fixed when ensureLinear is fixed.

Requesting re-review since I rewrote the patch in light of bug 890968

Comment on attachment 773268 [details] [diff] [review] Part 2: Add ParallelSpew intrinsic v2 Review of attachment 773268 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/SelfHosting.cpp @@ +346,5 @@ > + if (!str->getCharsNonDestructive(tcx, chars)) > + return false; > + range = TwoByteChars(chars, str->length()); > + } > + } Might be worth taking this big block of code making a helper so that one can write TwoByteChars range(str.threadSafeRange(tcx)) or something like that.

https://hg.mozilla.org/mozilla-central/rev/4d72769b6e9b https://hg.mozilla.org/mozilla-central/rev/068eacd6f3a3