Closed Bug 1320118 Opened 8 years ago Closed 8 years ago

Some CacheIR cleanup


(Core :: JavaScript Engine: JIT, defect, P1)




Tracking Status
firefox53 --- fixed


(Reporter: jandem, Assigned: jandem)


(Blocks 1 open bug)



(3 files)

There's some clean up I want to do after landing the patches that are currently in flight:

* The IR generator doesn't need to pass the writer to each tryAttach* method.

* The tryAttach methods can return an enum instead of using bool return value + bool emitted_.

* We can add a RAII class to emit stub frames.
Priority: -- → P1
Just for fun I tried using Result<bool> for the tryAttach methods, but I think it's confusing when "return false" suddenly isn't a failure, but just "not emitted". Additional it looks like right now none of the methods are actually failable, so we could just return a boolean for emitted/not emitted.
(In reply to Tom Schuster [:evilpie] from comment #1)
> Additional it looks like right now none of the methods are
> actually failable, so we could just return a boolean for emitted/not emitted.

Yeah, I was thinking along the same lines last night. It's also nice for the caller because it can just do:

  if (tryAttach1(..))
      return true;
  if (tryAttach2(..))
      return true;

I'm adding a fallible tryAttach in bug 1319437 (it calls the DOMProxyShadows hook), but I think it's nicer to just recover from OOM there and return not-emitted.
As discussed, this removes the emitted_ bool. The tryAttach* methods now return true only if they optimized the case.
Assignee: nobody → jdemooij
Attachment #8814663 - Flags: review?(evilpies)
We can now test CanAttachNone in the switch statement and remove the switch default.
Attachment #8814664 - Flags: review?(evilpies)
Comment on attachment 8814663 [details] [diff] [review]
Part 1 - Remove emitted_

Review of attachment 8814663 [details] [diff] [review]:

::: js/src/jit/CacheIR.h
@@ +522,5 @@
>      enum class PreliminaryObjectAction { None, Unlink, NotePreliminary };
>      PreliminaryObjectAction preliminaryObjectAction_;
> +    bool tryAttachNative(HandleObject obj, ObjOperandId objId);

I don't think keeping MOZ_MUST_USE would hurt.
Attachment #8814663 - Flags: review?(evilpies) → review+
Comment on attachment 8814664 [details] [diff] [review]
Part 2 - Simplify tryAttachNative

Review of attachment 8814664 [details] [diff] [review]:

::: js/src/jit/CacheIR.cpp
@@ +325,3 @@
>        case CanAttachCallGetter:
>          EmitCallGetterResult(writer, obj, holder, shape, objId);
> +        return true;

Nice that should give us warnings if we ever extend it.
Attachment #8814664 - Flags: review?(evilpies) → review+
This adds AutoStubFrame with enter/leave methods to manage stub frames.
Attachment #8814666 - Flags: review?(evilpies)
(In reply to Tom Schuster [:evilpie] from comment #5)
> I don't think keeping MOZ_MUST_USE would hurt.

Yeah I wasn't sure but I removed it because it could confuse people into thinking these methods return false on OOM.
Attachment #8814666 - Flags: review?(evilpies) → review+
Pushed by
part 1 - Remove emitted_ bool from GetPropIRGenerator. r=evilpie
part 2 - Clean up tryAttachNative to handle all cases in the switch statement. r=evilpie
part 3 - Introduce AutoStubFrame to emit stub frames. r=evilpie
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.