Some CacheIR cleanup

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
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.
(Assignee)

Comment 2

a year ago
(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.
(Assignee)

Comment 3

a year ago
Created attachment 8814663 [details] [diff] [review]
Part 1 - Remove emitted_

As discussed, this removes the emitted_ bool. The tryAttach* methods now return true only if they optimized the case.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8814663 - Flags: review?(evilpies)
(Assignee)

Comment 4

a year ago
Created attachment 8814664 [details] [diff] [review]
Part 2 - Simplify tryAttachNative

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+
(Assignee)

Comment 7

a year ago
Created attachment 8814666 [details] [diff] [review]
Part 3 - Introduce AutoStubFrame

This adds AutoStubFrame with enter/leave methods to manage stub frames.
Attachment #8814666 - Flags: review?(evilpies)
(Assignee)

Comment 8

a year ago
(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+

Comment 9

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12185bff2467
part 1 - Remove emitted_ bool from GetPropIRGenerator. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/88423aaff4da
part 2 - Clean up tryAttachNative to handle all cases in the switch statement. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1fa9c00c46
part 3 - Introduce AutoStubFrame to emit stub frames. r=evilpie

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12185bff2467
https://hg.mozilla.org/mozilla-central/rev/88423aaff4da
https://hg.mozilla.org/mozilla-central/rev/ee1fa9c00c46
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.