Closed Bug 1334187 Opened 7 years ago Closed 7 years ago

Port Baseline JSOP_IN to CacheIR

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

Product:
Core
Component:
JavaScript Engine: JIT
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 9 obsolete files)

Bug 1334187 - Add CacheIR scaffolding for JSOP_IN.
59 bytes, text/x-review-board-request
jandem
: review+
Details
Bug 1334187 - Add ICCacheIR_Regular base class.
59 bytes, text/x-review-board-request
jandem
: review+
Details
Bug 1334187 - Port In_Dense from BaselineIC to CacheIR.
59 bytes, text/x-review-board-request
jandem
: review+
Details
Bug 1334187 - Port In_Native/In_NativePrototype from BaselineIC to CacheIR.
59 bytes, text/x-review-board-request
jandem
: review+
Details
Bug 1334187 - Port In_NativeDoesNotExist from BaselineIC to CacheIR.
59 bytes, text/x-review-board-request
jandem
: review+
Details 
Port the Baseline ICs for JSOP_IN to CacheIR.
Assignee: nobody → tcampbell
Blocks: CacheIR
WIP. Ported In_Dense to CacheIR. I may refactor a bit as a finish the other JSOP_IN ICs.
Priority: -- → P3
Attachment #8831256 - Attachment is obsolete: true
Attachment #8831252 - Attachment is obsolete: true
Attachment #8831253 - Attachment is obsolete: true
Attachment #8831254 - Attachment is obsolete: true
Attachment #8831255 - Attachment is obsolete: true
Comment on attachment 8832986 [details]
Bug 1334187 - Add CacheIR scaffolding for JSOP_IN.

https://reviewboard.mozilla.org/r/109230/#review110618

Looks perfect.
Attachment #8832986 - Flags: review?(jdemooij) → review+
Comment on attachment 8832987 [details]
Bug 1334187 - Add CacheIR scaffolding for JSOP_IN.

https://reviewboard.mozilla.org/r/109232/#review110626

Looks good, r=me with nits below addressed.

::: js/src/jit/BaselineIC.cpp:2015
(Diff revision 2)
>  
>  static bool
>  DoInFallback(JSContext* cx, BaselineFrame* frame, ICIn_Fallback* stub_,
>               HandleValue key, HandleValue objValue, MutableHandleValue res)
>  {
> +    SharedStubInfo info(cx, frame, stub_->icEntry());

JSOP_IN does not use shared stubs so this isn't necessary (and we will likely remove shared stubs at some point). Instead of info.script() and info.outerScript(cx) below you can use frame->script().

::: js/src/jit/BaselineIC.cpp:2029
(Diff revision 2)
>          return false;
>      }
>  
> +    bool attached = false;
> +
> +    if (stub->numOptimizedStubs() >= ICIn_Fallback::MAX_OPTIMIZED_STUBS) {

Nit: no {} if each of condition, then-body, else-body fits on a single line.

::: js/src/jit/CacheIR.h:1006
(Diff revision 2)
> +    HandleValue key_;
> +    HandleObject obj_;
> +
> +  public:
> +    InIRGenerator(JSContext* cx, jsbytecode* pc,
> +                  HandleValue key, HandleObject obj);

Nit: this should fit on one line I think? The limit is max 99 chars for code, 79 for comments.

::: js/src/jit/CacheIR.cpp:1548
(Diff revision 2)
>      writer.typeMonitorResult();
>      return true;
>  }
>  
> +InIRGenerator::InIRGenerator(JSContext* cx, jsbytecode* pc,
> +                             HandleValue key, HandleObject obj)

Same nit here regarding line length.
Attachment #8832987 - Flags: review?(jdemooij) → review+
Sorry I have to run now, I'll finish reviewing this on Monday. Nice work so far!
Attachment #8832987 - Attachment is obsolete: true
Attachment #8832988 - Attachment is obsolete: true
Attachment #8832988 - Flags: review?(jdemooij)
Attachment #8832989 - Attachment is obsolete: true
Attachment #8832989 - Flags: review?(jdemooij)
Attachment #8832990 - Attachment is obsolete: true
Attachment #8832990 - Flags: review?(jdemooij)
Oops. Screwed up the Reviewboard state. Fixed the comments in second patch. The rest are unchanged still.
Comment on attachment 8833362 [details]
Bug 1334187 - Add ICCacheIR_Regular base class.

https://reviewboard.mozilla.org/r/109612/#review111098
Attachment #8833362 - Flags: review?(jdemooij) → review+
Comment on attachment 8833363 [details]
Bug 1334187 - Port In_Dense from BaselineIC to CacheIR.

https://reviewboard.mozilla.org/r/109614/#review111108

Looks great, just one maybeGuardInt32Index issue that's really a pre-existing problem.

::: js/src/jit/CacheIR.cpp:1559
(Diff revision 1)
> +                                HandleObject obj, ObjOperandId objId)
> +{
> +    uint32_t index;
> +    Int32OperandId indexId;
> +    if (!maybeGuardInt32Index(key, keyId, &index, &indexId))
> +        return false;

One issue here is that maybeGuardInt32Index may emit IR, so if we return true here and then one of the conditions below is false, we will return and then another tryAttach method may emit more IR into the buffer (for instance a second IsInt32Index op).

We ran into similar problems before and I think the nicest fix is to split maybeGuardInt32 into two methods:

bool isInt32Index(Value v, uint32_t* index);

Int32OperandId emitIsInt32Index(Value v, ValOperandId v, ...) {
    MOZ_ASSERT(isInt32Index(v));
}

That way we can check isInt32Index first, then after all conditions are checked we emit the IR for it. It's fine to keep the patch like this and post another one for this refactoring.
Attachment #8833363 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #31)
> Int32OperandId emitIsInt32Index(Value v, ValOperandId v, ...) {

emitGuardInt32Index may be nicer.
Comment on attachment 8833364 [details]
Bug 1334187 - Port In_Native/In_NativePrototype from BaselineIC to CacheIR.

https://reviewboard.mozilla.org/r/109616/#review111116

::: js/src/jit/CacheIR.cpp:44
(Diff revision 1)
>      isTemporarilyUnoptimizable_(isTemporarilyUnoptimizable),
>      canAttachGetter_(canAttachGetter),
>      preliminaryObjectAction_(PreliminaryObjectAction::None)
>  {}
>  
> +static void EmitNameOrSymbolGuard(CacheIRWriter& writer, jsid id, ValOperandId valId)

Sorry, I just landed a patch that does a similar refactoring (it adds IRGenerator::emitIdGuard IIRC).

::: js/src/jit/CacheIR.cpp:378
(Diff revision 1)
>          writer.guardShape(objId, shape);
>      }
>  }
>  
>  static void
> +EmitReadSlotGuard(CacheIRWriter& writer, JSObject* obj, JSObject* holder,

This function is very similar to EmitReadSlotResult. It might be nice to move most of EmitReadSlotResult into a new function, EmitReadSlotGuard, that returns the holder ObjOperandId. Either as return value or (optional/nullable) outparam.

Then EmitReadSlotResult can call EmitReadSlotGuard and the JSOP_IN code can also call EmitReadSlotGuard.

::: js/src/jit/CacheIRCompiler.cpp:1451
(Diff revision 1)
> +    if (output.hasValue()) {
> +        Value val = BooleanValue(reader.readBool());
> +        masm.moveValue(val, output.valueReg());
> +    }
> +    else
> +        masm.assumeUnreachable("Should have monitored result");

Nit: else branch should have {} if the then-block has them.

The (future) Ion IC for this will likely have a typed (MIRType::Bool) output register so we would have to implement this case. It doesn't matter for Baseline so it's probably best to change it to:

else {
    MOZ_CRASH("NYI: Typed LoadBooleanResult");
}
Attachment #8833364 - Flags: review?(jdemooij)
Comment on attachment 8833365 [details]
Bug 1334187 - Port In_NativeDoesNotExist from BaselineIC to CacheIR.

https://reviewboard.mozilla.org/r/109618/#review111122

That +28/-296 diffstat is excellent :)

::: js/src/jit/BaselineIC.cpp:1932
(Diff revision 1)
>      if (stub.invalid())
>          return true;
>  
>      if (attached)
>          return true;

Nit: these 2 if-statements can now be removed too \o/

::: js/src/jit/CacheIR.cpp:1644
(Diff revision 1)
> +InIRGenerator::tryAttachNativeInDoesNotExist(HandleValue key, ValOperandId keyId,
> +                                             HandleObject obj, ObjOperandId objId)
> +{
> +    RootedId id(cx_);
> +    bool nameOrSymbol;
> +    if (!ValueToNameOrSymbolId(cx_, key, &id, &nameOrSymbol)) {

We can move this into tryAttachStub, so we have to do this only once (and simplify tryAttachNativeIn and tryAttachNativeInDoesNotExist). Then we can pass the jsid instead of the key, something like:

if (nameOrSymbol) {
    if (tryAttachNativeIn(id, keyId, obj, objId))
        return true;
    if (tryAttachNativeInDoesNotExist(id, keyId, obj, objId)
        return true;
}
return false;
Attachment #8833365 - Flags: review?(jdemooij) → review+
Comment on attachment 8833363 [details]
Bug 1334187 - Port In_Dense from BaselineIC to CacheIR.

https://reviewboard.mozilla.org/r/109614/#review111108

> One issue here is that maybeGuardInt32Index may emit IR, so if we return true here and then one of the conditions below is false, we will return and then another tryAttach method may emit more IR into the buffer (for instance a second IsInt32Index op).
> 
> We ran into similar problems before and I think the nicest fix is to split maybeGuardInt32 into two methods:
> 
> bool isInt32Index(Value v, uint32_t* index);
> 
> Int32OperandId emitIsInt32Index(Value v, ValOperandId v, ...) {
>     MOZ_ASSERT(isInt32Index(v));
> }
> 
> That way we can check isInt32Index first, then after all conditions are checked we emit the IR for it. It's fine to keep the patch like this and post another one for this refactoring.

Left maybeGuardInt32 alone for now, but refactored the tryAttach method.
jandem: Updated with your feedback and rebased onto m-c. Try is running at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f76aad2b732
If things look good to you, perhaps you can Autoland it in review board for me. (Otherwise I need to checkin-needed)

Thanks!
Comment on attachment 8833364 [details]
Bug 1334187 - Port In_Native/In_NativePrototype from BaselineIC to CacheIR.

https://reviewboard.mozilla.org/r/109616/#review111564

Great!
Attachment #8833364 - Flags: review?(jdemooij) → review+
(In reply to Ted Campbell [:tcampbell] from comment #41)
> If things look good to you, perhaps you can Autoland it in review board for
> me. (Otherwise I need to checkin-needed)

I'm not familiar with MozReview/autoland, but we still need to change r?jandem to r=jandem right?

Also Automation -> Land Commits in MozReview does nothing for me, but I do see a tooltip saying I need scm_level_3 access to land.
(In reply to Jan de Mooij [:jandem] from comment #43)
> I'm not familiar with MozReview/autoland, but we still need to change
> r?jandem to r=jandem right?
Autoland rewrites the commit message to change r? to r=.

> Also Automation -> Land Commits in MozReview does nothing for me, but I do
> see a tooltip saying I need scm_level_3 access to land.

Probably because your L3 is tied to your GMail account and BMO is tied to Mozilla. Try following the directions below:
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html#manually-associating-your-ldap-account-with-mozreview
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/69f3c3a9a89e
Add ICCacheIR_Regular base class. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3b7fff9faaa3
Add CacheIR scaffolding for JSOP_IN. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8970424593bb
Port In_Dense from BaselineIC to CacheIR. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3a9e80340a04
Port In_Native/In_NativePrototype from BaselineIC to CacheIR. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ce4cea9734cb
Port In_NativeDoesNotExist from BaselineIC to CacheIR. r=jandem
(In reply to Ryan VanderMeulen [:RyanVM] from comment #44)
> Probably because your L3 is tied to your GMail account and BMO is tied to
> Mozilla.

Thanks! That fixed it.

(Btw I'm afraid using autoland for CacheIR changes will introduce conflicts when merging with inbound, but I don't think there are any changes on inbound atm so it should be fine now.)
(In reply to Jan de Mooij [:jandem] from comment #46)
> Thanks! That fixed it.

Great, glad to see it worked!

> (Btw I'm afraid using autoland for CacheIR changes will introduce conflicts
> when merging with inbound, but I don't think there are any changes on
> inbound atm so it should be fine now.)

Yeah, that's a valid concern. In general, if there's a merge conflict, tie goes to autoland (i.e. the conflicting patch on inbound will be backed out).
Good to know. Presumably we can merge directly from the review to m-i. Either way I still need someone to do my final check-in.
It's possible (importing the patches from the review repo into a local inbound clone), but generally not done. I think the general answer you're going to get is "use MozReview/Autoland more" :)
According to AWFY there's a small regression on Kraken:

https://arewefastyet.com/#machine=29&view=single&suite=kraken&subtest=crypto-pbkdf2

Regression range is the central -> inbound merge so it could be another bug, but considering the pbkdf2 test uses JSOP_IN it's probably this one :)

To repro locally you can clone https://github.com/h4writer/arewefastyet, then go to benchmarks/kraken and run something like this (Kraken uses the Sunspider harness):

$ ./sunspider --shell=<path to your shell> --test=pbkdf2

Let me know if you need any help.
Flags: needinfo?(tcampbell)
Confirmed. The slowdown is from my patch series. Most likely a stub is no longer being attached. I think will be related to Bug 1337763.
Flags: needinfo?(tcampbell)
Depends on: 1337811
(In reply to Ted Campbell [:tcampbell] from comment #52)
> Confirmed. The slowdown is from my patch series. Most likely a stub is no
> longer being attached. I think will be related to Bug 1337763.

I made a mistake in my analysis. Once I get rid of background noise, I don't see the regression. My tests are on 32-bit which doesn't have a regression on AWFY, so I will look into 64-bit.
I've been poking around at this and am unable to reproduce. Looking at the link, I see the spike in range dde8ef8cf38e..497e7f87f137 that you noticed. I've built Win32 and Win64 builds and cannot see a difference in perf between the two points. When I use JITFLAGS=bl-ic I get the same results. Only the 64-bit platform shows the slowdown, and it does recover a day later, so I'm starting to think it was a temporary slowdown of the host machine.
OK, sorry for the false alarm and thanks for investigating!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: