Closed Bug 730234 Opened 10 years ago Closed 10 years ago

Keep the GC lock only for background thread synchronization

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(1 file, 3 obsolete files)

With single threading runtime we should delegate any necessary locking that the embedding may need while implementing the operation and activities callbacks to the embedding itself. Clearly we must still allow to trigger operation callback calls from any thread, but that does not require any locking. The current JS_ATOMIC_SET in JSRuntime::triggerOperationCallback should cover all the necessary use cases.
Attached patch v1 (obsolete) — Splinter Review
The non-trivial part of the patch is introduction of own lock for the JS watchdog thread in XPCJSRuntime.cpp. The rest is the removal of no longer necessary AutoLockGC and stopping exposing it trough jsfriendapi.cpp.
Attachment #600349 - Flags: review?(jorendorff)
Attached patch v2 (obsolete) — Splinter Review
The patch required a rebase.
Attachment #600349 - Attachment is obsolete: true
Attachment #601428 - Flags: review?(jorendorff)
Attachment #600349 - Flags: review?(jorendorff)
Attached patch v3 (obsolete) — Splinter Review
Jason, do you have time to review this?
Attachment #601428 - Attachment is obsolete: true
Attachment #603053 - Flags: review?(jorendorff)
Attachment #601428 - Flags: review?(jorendorff)
Summary: remove GC locking from activities and operation callbacks-related code → Keep the GC lock only for background thread synchronization
Attached patch v4Splinter Review
The patch removes GC locking from the activity and operation callbacks, when accessing JSContext list in the runtime and when manipulating global roots. After that the GC lock remains only for the background thread synchronization. 

Even with the patch the locking is still extensive since in many cases the GC may just skip the lock if the background thread is idle. I will fix that in GC-related patch.

Luke, can you review the patch? Jason seems to be busy.
Attachment #603053 - Attachment is obsolete: true
Attachment #603703 - Flags: review?(luke)
Attachment #603053 - Flags: review?(jorendorff)
Comment on attachment 603703 [details] [diff] [review]
v4

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

Awesome.  Given that the "GC lock" now really only protects the background sweep thread, could you push the lock and locking calls down into GCHelperThread?  (Perhaps this is what you were going to do in the followup patch.)

::: js/src/jsgc.cpp
@@ +1774,5 @@
>      return false;
>  }
>  
>  void
>  js_UnlockGCThingRT(JSRuntime *rt, void *thing)

Speaking of, can these functions be removed?
Attachment #603703 - Flags: review?(luke) → review+
Blocks: 722345
Blocks: 734250
Blocks: 734255
https://hg.mozilla.org/mozilla-central/rev/dcb6daea6aef
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.