Closed
Bug 659241
Opened 13 years ago
Closed 13 years ago
IonMonkey: Stop passing JSContext * everywhere
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: adrake, Assigned: adrake)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
36.90 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Use a NSPR-provided TLS variable to hang onto the allocation pool and just use that.
Comment 1•13 years ago
|
||
We should do this change in the main tree.
Assignee | ||
Comment 2•13 years ago
|
||
$ 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.
Assignee | ||
Comment 4•13 years ago
|
||
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
Assignee | ||
Comment 5•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
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)
Comment 7•13 years ago
|
||
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?)
Comment 10•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
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.
Description
•