Closed Bug 1336603 Opened 6 years ago Closed 6 years ago

Move PCCounts state from ZoneGroup back to the runtime

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
Bug 1325050 moved the PCCounts state (|profilingScripts|, |scriptAndCountsVector|) from JSRuntime to ZoneGroup.  While this is nice for guaranteeing a preemptively scheduled thread exclusive access to this state, it isn't really compatible with the PCCounts API, which expects exclusive access to the entire runtime and wants counts in the form of a single runtime wide vector.  Rather than reworking this API, it seems simpler to just go back to having the state on the runtime for now, since there isn't a problem with synchronization here in a cooperatively scheduled runtime.

Most of this patch is backing out pieces of bug 1325050.  Sorry about the churn.
Attachment #8833513 - Flags: review?(jdemooij)
Comment on attachment 8833513 [details] [diff] [review]
patch

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

When we move to running JS concurrently, will the ProtectedData asserts catch problems here?
Attachment #8833513 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #1)
> Comment on attachment 8833513 [details] [diff] [review]
> patch
> 
> Review of attachment 8833513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When we move to running JS concurrently, will the ProtectedData asserts
> catch problems here?

Yes, before we can have preemptive multithreading we'll need to remove ActiveThreadData and all other stuff that depends on having an active thread on the runtime, replacing the synchronization methods on these fields with something that behaves correctly with concurrently running JS.
https://hg.mozilla.org/mozilla-central/rev/392e110a9769
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.