Last Comment Bug 770200 - GC: Simplify use of GC lock
: GC: Simplify use of GC lock
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: Jon Coppeard (:jonco)
:
:
Mentors:
Depends on:
Blocks: 729760
  Show dependency treegraph
 
Reported: 2012-07-02 08:54 PDT by Jon Coppeard (:jonco)
Modified: 2012-07-04 09:50 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Possible fix (8.76 KB, patch)
2012-07-02 09:07 PDT, Jon Coppeard (:jonco)
wmccloskey: review+
Details | Diff | Splinter Review
Fix for test failure on try (9.53 KB, patch)
2012-07-03 09:34 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review

Description Jon Coppeard (:jonco) 2012-07-02 08:54:52 PDT
There's quite a bit of code scattered throughout jsgc.cpp to take/release the GC lock, but this is mostly unnecessary now that only a single thread per runtime is allowed.

The locking code can be mostly factored into the functions which interact with the background thread, simplifying the code.
Comment 1 Jon Coppeard (:jonco) 2012-07-02 09:07:50 PDT
Created attachment 638386 [details] [diff] [review]
Possible fix

Possible fix.

The GC lock is still used for DecommitArenas, and which I think is necessary but I'm not sure about.
Comment 2 Gregor Wagner [:gwagner] 2012-07-02 09:22:47 PDT
That seems wrong. The lock is still needed because it's used for synchronization with the helperThread.
Comment 3 Bill McCloskey (:billm) 2012-07-02 10:47:37 PDT
Comment on attachment 638386 [details] [diff] [review]
Possible fix

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

Thanks, Jon. This looks great to me.

Gregor, the lock isn't being removed here. We're just simplifying how it's acquired and released. For example, the patch acquires the lock in waitBackgroundSweepEnd, instead of in all its callers.
Comment 4 Gregor Wagner [:gwagner] 2012-07-02 13:00:30 PDT
Oh right. I should have looked closer.
Comment 5 Jon Coppeard (:jonco) 2012-07-03 09:34:49 PDT
Created attachment 638768 [details] [diff] [review]
Fix for test failure on try

Fix for the following failure on the try server:

PROCESS-CRASH | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_5/extensions/regress-336409-2.js | application crashed (minidump found)

This was because JSRuntime::onOutOfMemory takes the GC lock before calling waitBackgroundSweepOrAllocEnd().
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-07-04 09:50:31 PDT
https://hg.mozilla.org/mozilla-central/rev/ed582ea30746

Note You need to log in before you can comment on or make changes to this bug.