Add ability to get and set per-Zone user data, and iterate over Zones

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed, firefox28 fixed, firefox-esr24 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments, 7 obsolete attachments)

This would be useful to have.  I was just going to add a void* to Zone, then a basic API to get and set it from the browser.  The name suggestion is Luke's but I can change it to whatever people think is good.
Comment on attachment 796099 [details] [diff] [review]
part 1: add getter and setter for per-Zone user data

I mostly just cargo-culted this from compartment privates.  Any and all naming suggestions welcome.  I'm going to wait to land this until I have something that actually uses it, so no big hurry on the review.
Attachment #796099 - Flags: review?(wmccloskey)
Comment on attachment 796099 [details] [diff] [review]
part 1: add getter and setter for per-Zone user data

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

This seems fine. Are you going to need a callback when the zone is destroyed?
Attachment #796099 - Flags: review?(wmccloskey) → review+
Good point.  If I malloc something to store as data, then I'll need a callback to free whatever is in there...
mccr8, any news here?
Flags: needinfo?(continuation)
I'll do this and bug 905382 next week when I get back from PTO.  Feel free to grab it in the mean time if you are bored. ;)
Flags: needinfo?(continuation)
Summary: Add js::(Get|Set)ZoneUserData to JS friend API → Add ability to get and set per-Zone user data, and iterate over Zones
Attachment #796099 - Attachment description: add it → part 1: add getter and setter for per-Zone user data
This adds the ability to set a callback for zone destruction, akin to the one for compartments. The callback only takes the zone as an argument, in comparison to the compartment one which takes a JSFreeOp, but at least for browser purposes we don't need the JSFreeOp for either.  I can add it in if you think it would be better.
This adds a way to iterate over all zones, similar to JS_IterateCompartments. As with part 2, I only need to know the zone, not a closure argument or the runtime, so I just left those arguments off the callback, but if you think it would be more futureproof to include those, I can add them.
I forgot to initialize the callback.
Attachment #810125 - Attachment is obsolete: true
Comment on attachment 810129 [details] [diff] [review]
part 3: implement JS_IterateZones

Does this look right to you, Bill?  I'm getting some wonky behavior with a patch that uses it, though it is probably unrelated to this.  I just mimicked JS_iterateCompartments here.  (Also feel free to review it at the same time if you want, but no hurry there.)
Attachment #810129 - Flags: feedback?(wmccloskey)
Comment on attachment 810129 [details] [diff] [review]
part 3: implement JS_IterateZones

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

Yes, this seems fine.

::: js/src/gc/Iteration.cpp
@@ +116,5 @@
> +{
> +    JS_ASSERT(!rt->isHeapBusy());
> +
> +    AutoTraceSession session(rt);
> +    rt->gcHelperThread.waitBackgroundSweepOrAllocEnd();

This line shouldn't be needed. The one in JS_IterateCompartments also seems unnecessary. Igor added it a while ago with no explanation. I would just take them both out.
Attachment #810129 - Flags: feedback?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #11)
> The one in JS_IterateCompartments also seems unnecessary. 

I split this removal out into bug 924443.
Attachment #810187 - Flags: review?(wmccloskey)
Attachment #810187 - Flags: review?(wmccloskey)
Attachment #810187 - Attachment is obsolete: true
Comment on attachment 814434 [details] [diff] [review]
part 2: add callback for zone destruction

I needed to change NULL to nullptr.
Attachment #814434 - Flags: review?(wmccloskey)
Attachment #814434 - Flags: review?(wmccloskey) → review+
This just uses nullptr instead of NULL.  Carrying forward billm's r+.
Attachment #796099 - Attachment is obsolete: true
Attachment #824110 - Flags: review+
This removes the call to waitBackgroundSweepOrAllocEnd and adds |AutoPauseWorkersForTracing pause(rt);| which JS_IterateCompartments seems to do now.  Carrying forward billm's r+.
Attachment #810129 - Attachment is obsolete: true
Attachment #824113 - Flags: review+
The AutoPause thing isn't necessary here, and it's going to be removed entirely soon.
Thanks, I'll remove it before landing.
I removed the pause thing.
Attachment #824113 - Attachment is obsolete: true
Comment on attachment 825954 [details] [diff] [review]
part 3: implement JS_IterateZones

carrying forward billm's r+
Attachment #825954 - Flags: review+
This just renames "JSDestroyZoneCallback" to "JSZoneCallback" so we can use it for other purposes.  Carrying forward billm's r+.
Attachment #814434 - Attachment is obsolete: true
Attachment #825954 - Attachment is obsolete: true
Attachment #827044 - Flags: review+
Comment on attachment 827046 [details] [diff] [review]
part 3: Add callback for zone sweeping

The idea here is that if we have a weakly-held cache stored on a zone, then we want to clear that cache before sweeping the zone.
Attachment #827046 - Flags: review?(jcoppeard)
Attachment #827046 - Flags: review?(jcoppeard) → review+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.