Keep the GC lock only for background thread synchronization

RESOLVED FIXED in mozilla13

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

(Blocks 1 bug)

unspecified
mozilla13
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Posted 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)
Posted 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)
Posted 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
Posted 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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.