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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

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 1

[Andrew McCreight [:mccr8]]

Attachment: part 1: add getter and setter for per-Zone user data

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 4

[Andrew McCreight [:mccr8]]

Good point.  If I malloc something to store as data, then I'll need a callback to free whatever is in there...

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

mccr8, any news here?

Comment 6

[Andrew McCreight [:mccr8]]

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. ;)

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 2: add callback for zone destruction

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.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 3: implement JS_IterateZones

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.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 2: add callback for zone destruction

I forgot to initialize the callback.

Comment 10

[Andrew McCreight [:mccr8]]

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.)

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 12

[Andrew McCreight [:mccr8]]

(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.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: part 2: add callback for zone destruction

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 814434 [details] [diff] [review]
part 2: add callback for zone destruction

I needed to change NULL to nullptr.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: part 1: add getter and setter for per-Zone user data

This just uses nullptr instead of NULL.  Carrying forward billm's r+.

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: part 3: implement JS_IterateZones

This removes the call to waitBackgroundSweepOrAllocEnd and adds |AutoPauseWorkersForTracing pause(rt);| which JS_IterateCompartments seems to do now.  Carrying forward billm's r+.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The AutoPause thing isn't necessary here, and it's going to be removed entirely soon.

Comment 18

[Andrew McCreight [:mccr8]]

Thanks, I'll remove it before landing.

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: part 3: implement JS_IterateZones

I removed the pause thing.

Comment 20

[Andrew McCreight [:mccr8]]

Comment on attachment 825954 [details] [diff] [review]
part 3: implement JS_IterateZones

carrying forward billm's r+

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: part 2: add callback for zone destruction

This just renames "JSDestroyZoneCallback" to "JSZoneCallback" so we can use it for other purposes.  Carrying forward billm's r+.

Comment 22

[Andrew McCreight [:mccr8]]

Attachment: part 3: Add callback for zone sweeping

Comment 23

[Andrew McCreight [:mccr8]]

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.

Comment 24

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/28b72504ea10
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/62ea644eaec8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/80c0c69444e2

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/28b72504ea10
https://hg.mozilla.org/mozilla-central/rev/62ea644eaec8
https://hg.mozilla.org/mozilla-central/rev/80c0c69444e2

Comment 26

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/cf6f49964224
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/9013fe5c2839
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/9576c40e9b23

Comment 27

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/27315aeda9dc
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b86f2eb6c072
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/005e933abd0b

Comment 28

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/releases/mozilla-esr24/rev/868026d87a6e
remote:   https://hg.mozilla.org/releases/mozilla-esr24/rev/e5cae78ef176
remote:   https://hg.mozilla.org/releases/mozilla-esr24/rev/4256f5910d80

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/cf6f49964224
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9013fe5c2839
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9576c40e9b23