Closed Bug 849469 Opened 11 years ago Closed 11 years ago

IonMonkey: Refactor GetNativePropertyStub

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: shu, Unassigned)

References

Details

Attachments

(1 file, 3 obsolete files)

Refactor GetNativePropertyStub by moving |generateReadSlot| into a base class inherited by both |GetPropertyIC| and |GetElementIC|.

Common arguments shared between |generateReadSlot| and |generateCallGetter|, and methods to generate exit and rejoin offsets are also refactored into a helper class, |GeneratePropertyStubHelper|.

The second refactoring is to prepare for bug 846111, which will use a non-repatching way to generate ICs that may be used in parallel execution.
Attached patch patch (obsolete) — Splinter Review
Attachment #723036 - Flags: review?(nicolas.b.pierron)
Blocks: 846111
Attached patch patch v2 (obsolete) — Splinter Review
Second iteration: refactors code having to do with patching jumps in the stubs into a StubPatcher class with one subclass, RepatchStubPatcher. The StubPatcher class encapsulates the various repatch labels and jumps. This is in preparation for bug 846111 which will introduce dispatch style stubs for parallel execution in addition to the current repatch style stubs.

Moves GenerateReadSlot into a static function in IonCaches.cpp.

Misc cleanup for generateCallGetter.
Attachment #723036 - Attachment is obsolete: true
Attachment #723036 - Flags: review?(nicolas.b.pierron)
Attachment #724733 - Flags: review?(nicolas.b.pierron)
Comment on attachment 724733 [details] [diff] [review]
patch v2

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

Great work, still a few things that I would like to see addressed. As you are in the area I would appreciate if you can remove the weird branchExit case of GenerateReadSlot in a separated path.

::: js/src/ion/IonCaches.cpp
@@ +108,5 @@
>  }
>  
>  const size_t IonCache::MAX_STUBS = 16;
>  
> +class IonCache::StubPatcher

I am not sure Patcher is the right term for this class as patching is only the action done when we attach the code.  I was thinking of something like Attacher / Connector / Plug / Insert … which means that with this class we handle the fact that the stub will be attached / connected / plugged / inserted to the rest of the ICs stubs.

@@ +116,5 @@
> +    bool hasStubCodePatchOffset_ : 1;
> +
> +    CodeLocationLabel rejoinLabel_;
> +
> +    RepatchLabel failures_;

Remove this RepatchLabel.

@@ +143,5 @@
> +
> +    template <class T1, class T2>
> +    void branchExit(MacroAssembler &masm, Assembler::Condition cond, T1 op1, T2 op2) {
> +        exitOffset_ = masm.branchPtrWithPatch(cond, op1, op2, &failures_);
> +        hasExitOffset_ = true;

nit: Assert that it does not have an ExitOffset before.
nit: Create and bind the RepatchLabel here as done in jumpExit.

@@ +155,5 @@
> +
> +    void jumpExit(MacroAssembler &masm) {
> +        RepatchLabel exit;
> +        exitOffset_ = masm.jumpWithPatch(&exit);
> +        hasExitOffset_ = true;

nit: Assert that it does not have an ExitOffset before.

@@ +163,5 @@
> +    void bindFailures(MacroAssembler &masm) {
> +        masm.bind(&failures_);
> +    }
> +
> +    void pushStubCodePatch(MacroAssembler &masm) {

nit: replace "Patch" by "Pointer" in the function name.

@@ +172,5 @@
> +        // WARNING: This is not a marking issue since the stub IonCode won't be collected
> +        // WARNING: between the time it's called and when we get here, but it would fail
> +        // WARNING: if the IonCode object ever moved, since we'd be rooting a nonsense
> +        // WARNING: value here.
> +        // WARNING:

nit: This warning is not clear and should have been removed before … what it should mention is:

  // WARNING: This location will be patched with the pointer of the generated stub, such as
  // it can be marked when a call is made with this stub.  Be aware that ICs are not marked
  // and so this stub will only be kept alive iff it is on the stack at the time of the GC.
  // No ImmGCPtr is needed as the stubs are flushed on GC.

@@ +174,5 @@
> +        // WARNING: if the IonCode object ever moved, since we'd be rooting a nonsense
> +        // WARNING: value here.
> +        // WARNING:
> +        stubCodePatchOffset_ = masm.PushWithPatch(STUB_ADDR);
> +        hasStubCodePatchOffset_ = true;

nit: Assert that this flag is false before setting it to true.

@@ +183,5 @@
> +        CodeLocationJump rejoinJump(code, rejoinOffset_);
> +        PatchJump(rejoinJump, rejoinLabel_);
> +    }
> +
> +    void patchStubCode(MacroAssembler &masm, IonCode *code) {

nit: rename to patchStubCodePointer

@@ +196,5 @@
> +};
> +
> +const ImmWord IonCache::StubPatcher::STUB_ADDR = ImmWord(uintptr_t(0xdeadc0de));
> +
> +// Repatch-style stubs are daisy chained in such a fashion that when

daisy ?

@@ +199,5 @@
> +
> +// Repatch-style stubs are daisy chained in such a fashion that when
> +// generating a new stub, the previous stub's exit jump is patched to the
> +// entry of our new stub.
> +class RepatchStubPatcher : public IonCache::StubPatcher

nit: rename to AppendStub

@@ +434,5 @@
>  
> +    bool multipleFailureJumps = (nonRepatchFailures != NULL) && nonRepatchFailures->used();
> +    patcher.branchExit(masm, Assembler::NotEqual,
> +                       Address(object, JSObject::offsetOfShape()),
> +                       ImmGCPtr(obj->lastProperty()));

(1)

@@ +521,2 @@
>  
> +    if (obj != holder || multipleFailureJumps) {

nit: Use this condition on top of (1), to use either masm.branchPtr or patcher.branchExit.
In fact, we can even handle the case where "obj == holder && !multipleFailureJumps" as a fast path through this function if needed.
Attachment #724733 - Flags: review?(nicolas.b.pierron)
Attached patch patch v3 (obsolete) — Splinter Review
Applied comments. Also refactors two more aspects of stub attaching: multiple exit jumps and saving a scratch register. This addresses your concern about the weird path in |GenerateReadSlot| somewhat.
Attachment #724733 - Attachment is obsolete: true
Attachment #728882 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #3)

> @@ +196,5 @@
> > +};
> > +
> > +const ImmWord IonCache::StubPatcher::STUB_ADDR = ImmWord(uintptr_t(0xdeadc0de));
> > +
> > +// Repatch-style stubs are daisy chained in such a fashion that when
> 
> daisy ?
> 

'Daisy chain' is a common phrased used to mean to connect together linearly. http://en.wiktionary.org/wiki/daisy_chain
common phrase*
Comment on attachment 728882 [details] [diff] [review]
patch v3

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

Quick notes …

::: js/src/ion/IonCaches.cpp
@@ +171,5 @@
> +//
> +// This corresponds to:
> +//
> +//   Label *failures = attacher.getCommonExit();
> +//   attacher.branchExit(masm, ...);

branchExit should be renamed branchFail.  because the rejoin point is a successful exit of the stub and the "exit" is an unsuccessful exit case.

@@ +174,5 @@
> +//   Label *failures = attacher.getCommonExit();
> +//   attacher.branchExit(masm, ...);
> +//   ... emit IC-specific code ...
> +//   Label *popAndExit = attacher.pushReg(masm, scratch);
> +//   masm.branchX(..., popAndExit);

This function does not add any value, on the contrary, it add more complexity to the interpretation of the attacher which is only supposed to deal with branches which are referring to any external locations.
Attached patch patch v4Splinter Review
Undo the multiple exit / scratch register management.

Rename the 'exit' terminology to 'nextStub' to signify that the jump is to the next stub in the IC. It was also proposed that it be renamed to 'failure', but I also find that confusing as when generating stub there are sometimes multiple labels inside the stub named some variation of 'failure'.
Attachment #728882 - Attachment is obsolete: true
Attachment #728882 - Flags: review?(nicolas.b.pierron)
Attachment #729223 - Flags: review?(nicolas.b.pierron)
Comment on attachment 729223 [details] [diff] [review]
patch v4

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

Thanks for doing this refactoring, this was the missing part of the huge IC refactoring, and this refactoring also open the door for changing the way ICs are attached to the code, such as making attempts for testing if other kind of Stub insertions are better.

::: js/src/ion/IonCaches.cpp
@@ +202,5 @@
> +        rejoinOffset_ = masm.jumpWithPatch(&rejoin);
> +        masm.bind(&rejoin);
> +    }
> +
> +    void jumpNextStub(MacroAssembler &masm) {

Great naming!  That's less ambiguous than fail or exit :)

@@ +248,5 @@
> +
> +// Repatch-style stubs are daisy chained in such a fashion that when
> +// generating a new stub, the previous stub's nextStub jump is patched to the
> +// entry of our new stub.
> +class RepatchStubAppender : public IonCache::StubAttacher

nit : Remove Repatch from the name, this class is only patching once, even if the jumps are modified the term Repatch is not clear as patching will stand for jumps, you your subject is Stub. s/RepatchStubAppender/StubAppender/

@@ +507,5 @@
> +    // Guard on the shape of the object.
> +    attacher.branchNextStubOrLabel(masm, Assembler::NotEqual,
> +                                   Address(object, JSObject::offsetOfShape()),
> +                                   ImmGCPtr(obj->lastProperty()),
> +                                   multipleFailureJumps ? failures : NULL);

nit: You don't need this ternary operator.

@@ +531,5 @@
> +    // Fast path: single failure jump, no prototype guards.
> +    if (!multipleFailureJumps) {
> +        EmitLoadSlot(masm, holder, shape, object, output, scratchReg);
> +        if (restoreScratch)
> +            masm.pop(scratchReg);

nit: JS_ASSERT(!restoreScratch); ?
Attachment #729223 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Comment on attachment 729223 [details] [diff] [review]
> patch v4
> 
> Review of attachment 729223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for doing this refactoring, this was the missing part of the huge IC
> refactoring, and this refactoring also open the door for changing the way
> ICs are attached to the code, such as making attempts for testing if other
> kind of Stub insertions are better.

Thanks for the review; this was needed to prepare for the forthcoming dispatch-style parallel ICs.

> 
> @@ +248,5 @@
> > +
> > +// Repatch-style stubs are daisy chained in such a fashion that when
> > +// generating a new stub, the previous stub's nextStub jump is patched to the
> > +// entry of our new stub.
> > +class RepatchStubAppender : public IonCache::StubAttacher
> 
> nit : Remove Repatch from the name, this class is only patching once, even
> if the jumps are modified the term Repatch is not clear as patching will
> stand for jumps, you your subject is Stub.
> s/RepatchStubAppender/StubAppender/

15:36 < shu> nbp: i chose to keep the RepatchStubAppender name because it's contrasted with DispatchStubPrepender or something like that in the new dispatch table-style ICs for parallel execution
15:36 < shu> nbp: unless you have a better suggestion; i thought it important to distinguish repatch vs dispatch

> 
> @@ +507,5 @@
> > +    // Guard on the shape of the object.
> > +    attacher.branchNextStubOrLabel(masm, Assembler::NotEqual,
> > +                                   Address(object, JSObject::offsetOfShape()),
> > +                                   ImmGCPtr(obj->lastProperty()),
> > +                                   multipleFailureJumps ? failures : NULL);
> 
> nit: You don't need this ternary operator.

Ah, good catch.

> @@ +531,5 @@
> > +    // Fast path: single failure jump, no prototype guards.
> > +    if (!multipleFailureJumps) {
> > +        EmitLoadSlot(masm, holder, shape, object, output, scratchReg);
> > +        if (restoreScratch)
> > +            masm.pop(scratchReg);
> 
> nit: JS_ASSERT(!restoreScratch); ?

No, it's possible to need to restore the scratch register even here. But since there are no failure jumps, we only need to restore the scratch on success, before jumping to rejoin. The initial |branchNextStub| happens before any scratch regs are needed.
https://hg.mozilla.org/mozilla-central/rev/5a277db93666
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: