Closed Bug 659241 Opened 13 years ago Closed 13 years ago

IonMonkey: Stop passing JSContext * everywhere

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: adrake, Assigned: adrake)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Use a NSPR-provided TLS variable to hang onto the allocation pool and just use that.
We should do this change in the main tree.
$ grep "(JSContext *"  *.h *.cpp */*.h */*.cpp | wc -l
4242

$ grep "(cx[,)]"  *.h *.cpp */*.h */*.cpp | wc -l
9730

That looks like a fun patch, although I certainly would not object to the result!
This is just local to IonMonkey code for now, there are so many places we have to pass cx in and it's getting really ugly.
Attached patch Patch v0 (obsolete) — Splinter Review
This removes most passing around of MIRGenerator * and JSContext *. In places where the values are used frequently, they remain members of the relevant structures.

I haven't written the NSPR bits yet, as it doesn't appear that the IonMonkey tree builds with --enable-threadsafe.
Assignee: general → adrake
Status: NEW → ASSIGNED
Nevermind on the build comment, I failed to enable system NSPR and did not have an NSPR build in-tree. Patch with NSPR support forthcoming.
Attached patch Patch v1Splinter Review
Adds NSPR support. Builds and runs in shell on current IonMonkey tip with or without threadsafe.
Attachment #534699 - Attachment is obsolete: true
Attachment #534707 - Flags: review?(dvander)
Blocks: 658486
Before we make this change, we should verify that it fits with our current threading model and our future plans, and then document that reasoning.
What could make this change infeasible:
 (1) If we wanted to nest compiler invocations on the same thread (easy to fix).
 (2) If the TLS calls were too expensive.

Are there any other ways it could violate a threading model?
If contexts/compilation can migrate between threads, maybe.  (Suspend worker for GC or sync operation, thread reassignment takes place?)
Depends on: 659404
JS_SetContextThread asserts cx->outstandingRequests (which is not set to zero by JS_SuspendRequest) is 0.  Calling JS_SetContextThread is a prerequisite for running a context on a different thread, thus a context should not being able to change threads during compilation, GC or no.
Comment on attachment 534707 [details] [diff] [review]
Patch v1

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

Great patch, I'm excited to try this model out. I did a quick silly benchmark: a script that generates ~1,500 IR instructions, each having two inputs, so that's about 1,500 * 3 allocations. The time in an optimized build went from 245,000ns to 256,000ns. So that's about 7ns per IR and 2ns per allocation, and ~4-5% of compile time.

Since the compilation process is so slim right now - really, all it's doing is allocation - it's not a fair test. We'll see how it plays out. Just some nits -

::: js/src/ion/IonBuilder.cpp
@@ +95,5 @@
> +        PRStatus status = PR_NewThreadPrivateIndex(&IonTLSIndex, NULL);
> +        if (status != PR_SUCCESS)
> +            return false;
> +        IonTLSInitialized = true;
> +    }

I think to be truly safe we will need to have an InitializeIon() call somewhere in JS_NewRuntime(). I am not sure what would happen if there were multiple runtimes being created at the same time on multiple threads... but I'm not going to worry about that.

It would also be nice if we could extract this code into another file, like Ion.cpp or something.

@@ +102,5 @@
>      TempAllocator temp(&cx->tempPool);
> +    IonContext ictx(cx, &temp);
> +
> +    if (!SetIonContext(&ictx))
> +        return false;

Can the SetIonContext(NULL) be in IonContext's destructor? to get rid of...

@@ +112,5 @@
>      IonBuilder analyzer(cx, script, fun, temp, graph);
>      spew.enable("/tmp/ion.cfg");
>  
>      if (!analyzer.analyze())
> +        goto error;

this. Bonus points if it becomes IonContext::enter()

::: js/src/ion/MIR.cpp
@@ +229,3 @@
>  {
> +    // XXXadrake: JS_ASSERT(index >= CALLEE_SLOT && index < int32(gen->fun()->nargs));
> +    return new MParameter(index);

What happened here?

::: js/src/ion/TempAllocPolicy.h
@@ +50,4 @@
>  namespace js {
>  namespace ion {
>  
>  class TempAllocPolicy

We should probably rename this to IonAllocPolicy now.
Attachment #534707 - Flags: review?(dvander) → review+
http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/b4dc641d8484
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.