1320118 - Some CacheIR cleanup

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Tom S [:evilpie]]

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.

Comment 2

[Jan de Mooij [:jandem]]

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

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove emitted_

As discussed, this removes the emitted_ bool. The tryAttach* methods now return true only if they optimized the case.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Simplify tryAttachNative

We can now test CanAttachNone in the switch statement and remove the switch default.

Comment 5

[Tom S [:evilpie]]

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.

Comment 6

[Tom S [:evilpie]]

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.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Introduce AutoStubFrame

This adds AutoStubFrame with enter/leave methods to manage stub frames.

Comment 8

[Jan de Mooij [:jandem]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12185bff2467
https://hg.mozilla.org/mozilla-central/rev/88423aaff4da
https://hg.mozilla.org/mozilla-central/rev/ee1fa9c00c46