Closed
Bug 844882
Opened 11 years ago
Closed 11 years ago
Encapsulate ParallelArray fields
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: nmatsakis, Assigned: shu)
References
Details
(Whiteboard: [PJS][js:t])
Attachments
(5 files, 2 obsolete files)
7.09 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
8.09 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
9.12 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
21.76 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
2.48 KB,
patch
|
nmatsakis
:
review-
|
Details | Diff | Splinter Review |
We need to encapsulate the fields for self-hosted parallel arrays. They are currently user-exposed. We have to be quite careful how we do it because of the following requirements: 1. Fast access. Perhaps ameliorated by caching the values into local variables on function entry. 2. Parallelizable. The access needs to compile to something that can execute in parallel execution mode. This means that it cannot go into the VM. Our current technique is to "pre-register" the relevant fields as "defined fields" in ParallelArray TI objects. This means that ion compiles them in the most efficient way possible. The best solution seems like it would be to use private names, once those are implemented. This could probably fit right in with our existing work on TI. Another possibility is to introduce intrinsics for each of the parallel array fields. So instead of |pa.buffer| we would write |GetBuffer(pa)| and |SetBuffer(pa)|. These intrinsics could be specially inlined in ion. It's verbose and painful but effective. The type sets of calls to intrisics are effectively barriered, so this is not the most efficient TI integration but might work out well. A final suggestion that has been made is to use a WeakMap from each PA to its fields. This will not work (at least not right now) because it's not especially fast and also uses VM code that is not safe for parallel execution. Finally the TI integration is likely quite poor, as the buffers etc for all TIs will be conflated together into one map, though perhaps if we access them through an intrinsic we will simply get barriers as before. For the moment, the ParallelArray API is still in flux and we are not concerned with 100% encapsulation. Therefore, I have simply filed this bug and added a FIXME in ParallelArray.js.
Reporter | ||
Updated•11 years ago
|
Whiteboard: PJS
Updated•11 years ago
|
Whiteboard: PJS → [PJS][js:t]
Reporter | ||
Comment 1•11 years ago
|
||
We've sketched out a plan here, which involves adding intrinsics to get and set reserved slots of self-hosted objects. E.g., instead of `pa.buffer`, we'd write `GetReservedSlot(pa, 0)`. We can then wrap these with macros to make it more palatable to access (e.g., `BUFFER(pa)` and `SET_BUFFER(pa, x)`, or perhaps `buffer(pa)` and `setBuffer(pa, x)`). We will add inlining paths for `GetReservedSlot()` and friends to ion so that these compile into simple loads and stores. It is incumbent on the self-hosted code to ensure that the `pa` object is actually a `ParallelArray`! Note that we will require that the naive fallback paths (which already exist) do not access internal variables. This is acceptable, as they do not now. This allows us to use the naive fallback paths in the event that `this` is not a ParallelArray but rather a proxy. These functions will just be ordinary natives, so we will need a type guard on each access. This is slightly less efficient than what we have now, but it's probably no big deal, and of course this allows for encapsulation. It is also quite simple to implement.
Reporter | ||
Comment 2•11 years ago
|
||
Also, to eliminate the type guards, we can implement `GetImmutableReservedSlot`, which would be used only for slots that are never mutated once set. We could then have the `BUFFER(x)` macro use `GetImmutableReservedSlot` instead of `GetReservedSlot`.
Assignee | ||
Updated•11 years ago
|
Assignee: general → shu
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #738822 -
Flags: review?(tschneidereit)
Assignee | ||
Comment 4•11 years ago
|
||
After discussion with bhackett on IRC, integration with TI (that is, tracking reserved slot types like we track property types) was deemed unnecessary for now. We're going to eat the type barrier costs until it becomes a problem.
Attachment #738823 -
Flags: review?(jdemooij)
Comment 5•11 years ago
|
||
Comment on attachment 738822 [details] [diff] [review] Part 1: Intrinsics to set/get reserved slots Review of attachment 738822 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. At some point, we should probably introduce a SelfHosting.h and move the intrinsics declarations there. This isn't the patch for that, however. ::: js/src/builtin/ParallelArray.cpp @@ +162,5 @@ > // addDefiniteProperties() above should have added one > // property for each of the fixed slots: > JS_ASSERT(paTypeObject->getPropertyCount() == NumFixedSlots); > } > + */ Any reason this is part of the patch? Seems like it should either not exist at all (in that the commented code should be removed) or in another patch.
Attachment #738822 -
Flags: review?(tschneidereit) → review+
Updated•11 years ago
|
Attachment #738823 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #5) > Comment on attachment 738822 [details] [diff] [review] > Part 1: Intrinsics to set/get reserved slots > > Review of attachment 738822 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. > > At some point, we should probably introduce a SelfHosting.h and move the > intrinsics declarations there. This isn't the patch for that, however. > > ::: js/src/builtin/ParallelArray.cpp > @@ +162,5 @@ > > // addDefiniteProperties() above should have added one > > // property for each of the fixed slots: > > JS_ASSERT(paTypeObject->getPropertyCount() == NumFixedSlots); > > } > > + */ > > Any reason this is part of the patch? Seems like it should either not exist > at all (in that the commented code should be removed) or in another patch. Oops, that shouldn't be part of the patch. Got sloppy while refreshing.
Assignee | ||
Comment 7•11 years ago
|
||
Add a general mechanism for self-hosted classes so that we can do encapsulation entirely from within self-hosted code. This requires a little bit of extra work to free up the self-hosted classes when done. I tried to this via refcounting at first, but JSObject finalizers check for clasp->finalize, which is bad when the clasp might be dangling. So the current approach just deletes all self-hosted classes when we clean up the last context, *after* the last GC. What do you think of this approach, Till?
Attachment #740591 -
Flags: feedback?(tschneidereit)
Comment 8•11 years ago
|
||
Comment on attachment 740591 [details] [diff] [review] Part 3: Self-hosted classes Review of attachment 740591 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall. I don't particularly like that we have `finishSelfHosting()` followed by `deleteSelfHostedClasses()` two calls later, but I guess some renaming can fix that. Also, don't we need to trace the classes? Or are they traced by the objects they're the prototypes of?
Attachment #740591 -
Flags: feedback?(tschneidereit) → feedback+
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #8) > Comment on attachment 740591 [details] [diff] [review] > Part 3: Self-hosted classes > > Review of attachment 740591 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good overall. > > I don't particularly like that we have `finishSelfHosting()` followed by > `deleteSelfHostedClasses()` two calls later, but I guess some renaming can > fix that. I had meant to ask you about that. What is the purpose served by assigning selfHostingGlobal_ = NULL in finishSelfHosting()? It's just a JSObject *, so it doesn't look like unpinning a GC root. > > Also, don't we need to trace the classes? Or are they traced by the objects > they're the prototypes of? Classes aren't GC things. The reason for deleteSelfHostedClasses() is to free them after GC has finished, as freeing them as part of finalizers for the prototypes that are "instances" of these classes result in invalid reads (as explained in Comment 7). Since the prototypes that hold the classes alive are held alive by the self-hosting global until the end of the lifetime of the last JSContext anyways, these manually allocated Classes are just going to be alive until the end of the last JSContext.
Comment 10•11 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #9) > (In reply to Till Schneidereit [:till] from comment #8) > > Comment on attachment 740591 [details] [diff] [review] > > Part 3: Self-hosted classes > > > > Review of attachment 740591 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Looks good overall. > > > > I don't particularly like that we have `finishSelfHosting()` followed by > > `deleteSelfHostedClasses()` two calls later, but I guess some renaming can > > fix that. > > I had meant to ask you about that. What is the purpose served by assigning > selfHostingGlobal_ = NULL in finishSelfHosting()? It's just a JSObject *, so > it doesn't look like unpinning a GC root. That'll be bug 852789, which you might remember. We should be able to do that after the GC and fold `deleteSelfHostedClasses()` into `finishSelfHosting()`. > > > > > Also, don't we need to trace the classes? Or are they traced by the objects > > they're the prototypes of? > > Classes aren't GC things. The reason for deleteSelfHostedClasses() is to > free them after GC has finished, as freeing them as part of finalizers for > the prototypes that are "instances" of these classes result in invalid reads > (as explained in Comment 7). Since the prototypes that hold the classes > alive are held alive by the self-hosting global until the end of the > lifetime of the last JSContext anyways, these manually allocated Classes are > just going to be alive until the end of the last JSContext. Oh right, thanks for the refresher. I cargo-culted the class stuff way too much, I'm afraid.
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #740591 -
Attachment is obsolete: true
Attachment #743402 -
Flags: review?(tschneidereit)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #743403 -
Flags: review?(jdemooij)
Comment 13•11 years ago
|
||
Comment on attachment 743402 [details] [diff] [review] Part 3: Self-hosted classes Review of attachment 743402 [details] [diff] [review]: ----------------------------------------------------------------- Nice.
Attachment #743402 -
Flags: review?(tschneidereit) → review+
Comment 14•11 years ago
|
||
Comment on attachment 743403 [details] [diff] [review] Part 4: Self-hosted class intrinsic inlines Review of attachment 743403 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/MIR.h @@ +7344,5 @@ > }; > > +class MHaveSameClass > + : public MBinaryInstruction, > + public MixPolicy<ObjectPolicy<0>, ObjectPolicy<1> > You also have to add a typePolicy member: TypePolicy *typePolicy() { return this; } @@ +7363,5 @@ > + > + MDefinition *lhs() const { > + return getOperand(0); > + } > + MDefinition *rhs() const { Nit: MBinaryInstruction (from which this inherits) already defines these guys.
Attachment #743403 -
Flags: review?(jdemooij) → review+
Comment 15•11 years ago
|
||
Comment on attachment 743403 [details] [diff] [review] Part 4: Self-hosted class intrinsic inlines Review of attachment 743403 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/CodeGenerator.cpp @@ +6207,5 @@ > + Register rhs = ToRegister(ins->rhs()); > + Register output = ToRegister(ins->output()); > + > + masm.loadObjClass(lhs, lhs); > + masm.loadObjClass(rhs, rhs); Hm, just thought about this, you can't clobber lhs/rhs (instructions following this one will expect them to still hold the object pointers). Simplest fix is to use two temps for the two Class pointers.
Assignee | ||
Comment 16•11 years ago
|
||
Thanks for the catches, jandem. Carrying over r+.
Attachment #743403 -
Attachment is obsolete: true
Attachment #744986 -
Flags: review+
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #764531 -
Flags: review?(nmatsakis)
Reporter | ||
Updated•11 years ago
|
Attachment #764531 -
Flags: review?(nmatsakis) → review+
Reporter | ||
Comment 18•11 years ago
|
||
Comment on attachment 764531 [details] [diff] [review] Part 5: jit-tests Making getSelfHostedValue() always available messes with the fuzzer, I fear.
Attachment #764531 -
Flags: review+ → review-
Assignee | ||
Comment 19•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad7821d7de3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/abd234497f3c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4a8fcfd15c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c0519f3dac Pushed without tests due to comment 18.
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3ad7821d7de3 https://hg.mozilla.org/mozilla-central/rev/abd234497f3c https://hg.mozilla.org/mozilla-central/rev/3d4a8fcfd15c https://hg.mozilla.org/mozilla-central/rev/d4c0519f3dac
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee | ||
Comment 21•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d210ebcc3509 Pushed part 5 since Till landed bug 885361.
You need to log in
before you can comment on or make changes to this bug.
Description
•