If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Optimize JSOP_IN for baseline compiler

RESOLVED FIXED in Firefox 40

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ting, Assigned: ting)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40+ fixed)

Details

Attachments

(1 attachment, 14 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Traced ICGetProp_Native, it is attached from DoGetPropFallback() and the optimized code is generated from ICGetPropNativeCompiler::generateStubCode():

  DoGetPropFallback()
    TryAttachNativeGetPropStub()
      ICGetPropNativeCompiler::getStub()
        return ICGetProp_Native::New(..., getStubCode(), ...)
          ICStubCompiler::getStubCode()
            ICGetPropNativeCompiler::generateStubCode()
(Assignee)

Comment 2

3 years ago
Read the comment of BaselineIC.h, but don't get the idea of TypeMonitor and TypeUpdate ICs. Why monitor the resulting types of getter operations, and why mointor the input types of setter operations?
(Assignee)

Comment 3

3 years ago
I am wondering how do optimized stubs get called, not sure is it following this path:

  GetPropertyOperation()
    JSObject::getGeneric()
      js::baseops::GetProperty()
        GetPropertyHelperInline()
          NativeGetInline()
            Shape::get()
              js::InvokeGetterOrSetter()
                js::Invoke()
                  js::RunScript()
                    jit::EnterBaselineMethod()
                      EnterBaseline()
(In reply to Ting-Yu Chou [:ting] from comment #2)
> Read the comment of BaselineIC.h, but don't get the idea of TypeMonitor and
> TypeUpdate ICs. Why monitor the resulting types of getter operations, and
> why mointor the input types of setter operations?

That's a requirement for type-inference.  Type-inference associates TypeSets with both locations in the source code where values are returned (calls, getprops, getelems, etc.), as well as with slots on objects where values can be stored (setprop, setelem).  Whenver new values flow through these locations, the typeset needs to be updated to ensure that the type of the value is contained in the typeset (if it's not already contained).  That's what the TypeMonitor and TypeUpdate ICs are for.

In this case, TypeUpdate is not relevant since you're not storing anything into an object.  Also, TypeMonitor is not relevant because TI can statically determine that the result of an 'in' operator is always going to be a boolean.

So this is one way that the JSOP_IN optimized native stub will be different from the JSOP_GETPROP optimized native stub.  the JSOP_GETPROP stub NEEDS to monitor the value it returns.  JSOP_IN doesn't (since it'll always return a boolean value).



As for your second question, optimized stubs get called directly from baseline jitcode.  It's not useful to look at the C stack for this.

If you have time, we can videoconference and I can take you through some of the design of the baseline compiler and how things are organized.
(Assignee)

Comment 5

3 years ago
Thank you for your explanation, it helps a lot.

A video conference would be great, seems we have 12 hours time difference, I can do it from home, late night or early morning both fine for me.
This week is busy for me.  Can we do next week sometime?  Are you TingPing on #jsapi?  I can ping you there.
(Assignee)

Comment 7

3 years ago
Sure, whenever you are available, I am "ting" on IRC.
(Assignee)

Comment 8

3 years ago
- ICGetPropNativeCompiler remembers isFixedSlot_, isCallProp_, propName_, and offset_ for accessing the property, but I guess for In, remember the result is enough which the logic of generated stub would be something like below, is my understanding correct?

  if obj.shape == stub.shape || holder.shape == stub.holderShape
     return rememberedResult;
  else
     go to next stub

- In TryAttachNativeGetPropStub(), there're not only ICGetPropNativeCompiler, but also ICGetProp_CallScripted::Compiler, ICGetPropCallDOMProxyNativeCompiler, ICGetProp_CallNative::Compiler, etc. I am not sure are those all needed.
Hey Ting, tried to look for you on IRC the last couple days.  I guess our timezones aren't jiving.  Let's setup a time to meet here in the bug.  What TZ are you in?

About your questions:

The obj.shape == stub.shape && holder.shape == stub.holderShape check can only be used to return affirmative responses (i.e. when attaching the optimized stub, for native objects that hold the property on the chain, you do the check and return true).

For a negative response (returning false for properties that don't exist), you need to do a full walk of the proto chain and check the shape for each proto.  This is due to peculiarities of something called telescoping shape optimization, which I can explain in more detail.


About the GetProp and other fancier stubs - yeah, don't worry about those for now - most of the time when people are checking for properties with 'in', the property will either exist as a simple slot property, or not exist at all.
(Assignee)

Comment 10

3 years ago
I am in UTC+8.
(Assignee)

Comment 11

3 years ago
Created attachment 8476566 [details] [diff] [review]
wip

Implemented In_Native, In_NativeDoesNotExist, and In_NativePrototype since they're quite similiar.

I also got some questions while doing it:

- Is checking the key a string necessary like TryAttachNativeGetElemStub()?
- GetProp has different way from GetElem to convert the key to property name, not sure which is correct for In?
- There's a rooted |lastShape| which is not referenced in TryAttachNativeDoesNotExistStub(), why is it needed?
- Except DoInFallbackFn, are there anything else I need to change for adding an argument to DoInFallback()?

Thank you :)
Attachment #8476566 - Flags: review?(kvijayan)
Comment on attachment 8476566 [details] [diff] [review]
wip

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

Nice work!  There are a few fundamental issues to address, but you're well on your way there.  Aside from the review comments below, the big deficiency in the current patch is that the logic isn't handling the fact that the input key can differ each time.  The optimized stubs only check the shape of the object, without confirming that the property being checked is actually the same property or not.

So if we have a line of code like:
  x in foo

and x happens to be the string "a" the first time (and "a" happens to be defined on foo), this code will attach a stub that checks the shape of foo and returns true.  The next time the line is executed if x is a different property name that is NOT defined on foo, then the shape-checking stub will still execute.

All the stubs need to keep a HeapPtrPropertyName around, and the |generateStubCode| needs to be modified to check for that.  See the code in |ICGetElemNativeCompiler::generateStubCode| to get an idea for what the key-comparison code looks like.  For now, you can avoid the whole |if (needsAtomize_) { ... }| block in your implementation and just have it be a simple pointer-identity check.

::: js/src/jit/BaselineIC.cpp
@@ +5713,5 @@
> +        return false;
> +
> +    uint32_t dummy;
> +    if (!JSID_IS_ATOM(id) || JSID_TO_ATOM(id)->isIndex(&dummy))
> +        return true;

I'm just noticing that this sequence of:

 if (!key.isString()) ...
 if (!ValueToId<CanGC>(...)) ...
 if (!JS_ID_IS_ATOM(id) || ...) ...

sequence now shows up three times in BaselineIC.cpp.  It may be good to factor this out into a helper with the signature:

  static bool 
  IsOptimizableElementPropertyName(JSContext *cx, HandleValue key, MutableHandleId idOut);

@@ +6818,5 @@
>      }
>  
>      if (res.isUndefined()) {
>          // Try attaching property-not-found optimized stub for undefined results.
> +        if (!TryAttachNativeGetPropDoesNotExistStub(cx, script, pc, stub, name, val, &attached))

Nice.

::: js/src/jit/BaselineIC.h
@@ +3815,5 @@
> +// Base class for In_Native and In_NativePrototype stubs.
> +class ICInNativeStub : public ICStub
> +{
> +    // Object shape (lastProperty).
> +    HeapPtrShape shape_;

This base class needs to additionally store the property name being checked for.  We can't guarantee that the incoming property being checked for will always be the same, so whatever stub we generate will need to keep track of the property name it was generated for.

@@ +3829,5 @@
> +        return offsetof(ICInNativeStub, shape_);
> +    }
> +};
> +
> +// Stub for checking an own property on a native object.

Nit: note in the comment that the stub is for checking affirmative results only.  So you may wanna word this "Stub for -confirming- an own property on a native object - if the stub passes its guards, it will always return true."

@@ +3851,5 @@
> +};
> +
> +// Stub for checking a property on a native object's prototype. Note that due to
> +// the shape teleporting optimization, we only have to guard on the object's shape
> +// and the holder's shape.

Same nit as in comment above.
Attachment #8476566 - Flags: review?(kvijayan)
(Assignee)

Comment 13

3 years ago
Thanks Kannan, I'll address these.
(Assignee)

Comment 14

3 years ago
(In reply to Kannan Vijayan [:djvj] from comment #12)
> All the stubs need to keep a HeapPtrPropertyName around, and the
> |generateStubCode| needs to be modified to check for that.

I see, but why |ICGetPropNativeDoesNotExistCompiler::generateStubCode| doesn't check the property name?
Flags: needinfo?(kvijayan)
(Assignee)

Comment 15

3 years ago
Created attachment 8478159 [details] [diff] [review]
wip

(In reply to Kannan Vijayan [:djvj] from comment #12)
> ::: js/src/jit/BaselineIC.cpp
> @@ +5713,5 @@
> > +        return false;
> > +
> > +    uint32_t dummy;
> > +    if (!JSID_IS_ATOM(id) || JSID_TO_ATOM(id)->isIndex(&dummy))
> > +        return true;
> 
> I'm just noticing that this sequence of:
> 
>  if (!key.isString()) ...
>  if (!ValueToId<CanGC>(...)) ...
>  if (!JS_ID_IS_ATOM(id) || ...) ...
> 
> sequence now shows up three times in BaselineIC.cpp.  It may be good to
> factor this out into a helper with the signature:
> 
>   static bool 
>   IsOptimizableElementPropertyName(JSContext *cx, HandleValue key,
> MutableHandleId idOut);
> 

Added.

> ::: js/src/jit/BaselineIC.h
> @@ +3815,5 @@
> > +// Base class for In_Native and In_NativePrototype stubs.
> > +class ICInNativeStub : public ICStub
> > +{
> > +    // Object shape (lastProperty).
> > +    HeapPtrShape shape_;
> 
> This base class needs to additionally store the property name being checked
> for.  We can't guarantee that the incoming property being checked for will
> always be the same, so whatever stub we generate will need to keep track of
> the property name it was generated for.

Added, but not sure is it also required for ICIn_NativeDoesNotExist.

> 
> @@ +3829,5 @@
> > +        return offsetof(ICInNativeStub, shape_);
> > +    }
> > +};
> > +
> > +// Stub for checking an own property on a native object.
> 
> Nit: note in the comment that the stub is for checking affirmative results
> only.  So you may wanna word this "Stub for -confirming- an own property on
> a native object - if the stub passes its guards, it will always return true."
> 
> @@ +3851,5 @@
> > +};
> > +
> > +// Stub for checking a property on a native object's prototype. Note that due to
> > +// the shape teleporting optimization, we only have to guard on the object's shape
> > +// and the holder's shape.
> 
> Same nit as in comment above.

Updated.

Also updated |ICInNativeCompiler::generateStubCode| and |ICInNativeDoesNotExistCompiler::generateStubCode| a bit.
Attachment #8476566 - Attachment is obsolete: true
(In reply to Ting-Yu Chou [:ting] from comment #14)
> (In reply to Kannan Vijayan [:djvj] from comment #12)
> > All the stubs need to keep a HeapPtrPropertyName around, and the
> > |generateStubCode| needs to be modified to check for that.
> 
> I see, but why |ICGetPropNativeDoesNotExistCompiler::generateStubCode|
> doesn't check the property name?

Because for JSOP_GETPROP, the property name is always the same at a given site.  GetProp bytecode ops are only generated for code like the following:

  obj.foo

There will never be a property other than 'foo' accessed at that site, because foo is not a variable, but rather a static name that never chages.  On the other hand, JSOP_IN is generated for code like:

 foo in obj

Where |foo| is a variable whose value can be different for every invocation of the op.  If you look in js/src/vm/Opcodes.h, you'll notice that JSOP_GETPROP takes only 1 input (the value to get the property from), and JSOP_IN takes 2 inputs (the value to check for the existence of a property, and the property to check for).
Flags: needinfo?(kvijayan)
Comment on attachment 8478159 [details] [diff] [review]
wip

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

Looking pretty good, this seems almost ready to land.  Probably good enough to run through try after the fixes below.

::: js/src/jit/BaselineIC.cpp
@@ +3796,5 @@
>      }
>      return false;
>  }
>  
> +static bool 

Nit: remove whitespace at end of line.

@@ +5820,5 @@
> +    Register strExtract = masm.extractString(R0, ExtractTemp0);
> +    masm.loadPtr(Address(BaselineStubReg, ICInNativeStub::offsetOfName()), scratch);
> +    masm.branchPtr(Assembler::NotEqual, strExtract, scratch, &failure);
> +
> +    // Unbox and shape guard.

Nit: "Unbox and shape guard object."

@@ +5827,5 @@
> +    masm.branchTestObjShape(Assembler::NotEqual, objReg, scratch, &failure);
> +
> +    if (kind == ICStub::In_NativePrototype) {
> +        // Shape guard holder.
> +        Register holderReg = regs.takeAny();

There's a slight issue here.  On x86, you don't have enough registers for this:

R0 and R1 takes up eax, ebx, ecx, and edx.  The stub pointer takes edi, and the |scratch| register above takes esi.  ebp and esp are reserved for the frame and stack pointer.. so no regs left.

Usually what we do here is: Push R0.scratchReg() to stack, use holderReg = R0.scratchReg() just within the conditional, and then when successfully falling through, drop the value from the stack with an |masm.addPtr(ImmPtr(sizeof(size_t)), StackPointer)|.

The failure path also needs to be handled correctly, since you can't jump to |failure| without R0 being whole again (the next stub needs to be able to use R0).  For that, we add a secondary failure path that flows into the main failure path like so:

masm.bind(&failurePopR0Scratch);
masm.pop(R0.scratchReg());
masm.bind(&failure);
...

And then within the conditional jump to |failurePopR0Scratch| instead of |failure|.

@@ +5902,5 @@
> +    masm.loadPtr(Address(BaselineStubReg, ICIn_NativeDoesNotExist::offsetOfShape(0)),
> +                 scratch);
> +    masm.branchTestObjShape(Assembler::NotEqual, objReg, scratch, &failure);
> +
> +    Register protoReg = regs.takeAny();

You have to use the same trick as above here.  Push R0.scratchReg(), use THAT as your protoReg, and make sure that you drop it from the stack on teh success path (since you don't care about R0 on the success path), and that you pop it back into R0.scratchReg() on the failure path.

@@ +10584,5 @@
> +    name_(name)
> +{ }
> +
> +/* static */ ICIn_Native *
> +ICIn_Native::Clone(JSContext *cx, ICStubSpace *space, ICIn_Native &other)

You don't need this ::Clone method.  It's only necessary for stubs that may call into other functions.

@@ +10600,5 @@
> +    holderShape_(holderShape)
> +{ }
> +
> +/* static */ ICIn_NativePrototype *
> +ICIn_NativePrototype::Clone(JSContext *cx, ICStubSpace *space, ICIn_NativePrototype &other)

Ditto, this ::Clone method is not needed.

::: js/src/jit/BaselineIC.h
@@ +3906,5 @@
> +class ICInNativeCompiler : public ICStubCompiler
> +{
> +    HandleObject obj_;
> +    HandleObject holder_;
> +    HandlePropertyName name_;

These should be Rooted instead of Handle, as ICInNativeCompiler is stack-allocated, and not a temporary parameter argument.  So:  RootedObject, and RootedPropertyName.
(Assignee)

Comment 18

3 years ago
Thank you for explaining me patiently, I will fix those and probably add the dense elements optimization.
(Assignee)

Comment 19

3 years ago
(In reply to Kannan Vijayan [:djvj] from comment #17)
> ::: js/src/jit/BaselineIC.h
> @@ +3906,5 @@
> > +class ICInNativeCompiler : public ICStubCompiler
> > +{
> > +    HandleObject obj_;
> > +    HandleObject holder_;
> > +    HandlePropertyName name_;
> 
> These should be Rooted instead of Handle, as ICInNativeCompiler is
> stack-allocated, and not a temporary parameter argument.  So:  RootedObject,
> and RootedPropertyName.

Sorry for keep asking, but why ICGetPropNativeCompiler and ICGetElemNativeCompiler both use Handle?
(Assignee)

Updated

3 years ago
Flags: needinfo?(kvijayan)
(In reply to Ting-Yu Chou [:ting] from comment #19)
> Sorry for keep asking, but why ICGetPropNativeCompiler and
> ICGetElemNativeCompiler both use Handle?

Good question.  They shouldn't, as a matter of hygiene.  They should use Rooteds.  If you don't mind, can you file a separate bug to fix any issues with baseline IC compiler classes having Handle* member fields?  That is a decent beginner bug too.
Flags: needinfo?(kvijayan)
(Assignee)

Comment 21

3 years ago
(In reply to Kannan Vijayan [:djvj] from comment #20)
> If you don't mind, can you file a separate bug to fix any issues
> with baseline IC compiler classes having Handle* member fields?  That is a
> decent beginner bug too.

Created bug 1059044.
(Assignee)

Comment 22

3 years ago
One more question.

I just read RootingAPI.h, and double checked the code instantiate ICGetElemNativeCompiler. When it is instantiated, the passed-in |obj|, |holder| and |propName| are actually Rooted*, isn't that enough for the compiler? I thought the compiler will be destructed after it goes out of scope.
Flags: needinfo?(kvijayan)
(In reply to Ting-Yu Chou [:ting] from comment #22)
> One more question.
> 
> I just read RootingAPI.h, and double checked the code instantiate
> ICGetElemNativeCompiler. When it is instantiated, the passed-in |obj|,
> |holder| and |propName| are actually Rooted*, isn't that enough for the
> compiler? I thought the compiler will be destructed after it goes out of
> scope.

You're right, it's not a correctness issue technically.  But it is a hygiene issue.  Handles should be used for parameter passing - temporaries on the stack that refer to rooted values, but as soon as you're holding onto a value on the stack explicitly, you should be using Rooteds.  For example, you'll never see a place in the source code where we instantiate a Handle explicitly on the stack like so:

  HandleString stringHandle = ...;

It's always implicitly by passing stack-allocated Rooteds into function arguments, or by passing Handles that were passed in as arguments into callee function arguments.
Flags: needinfo?(kvijayan)
(Assignee)

Comment 24

3 years ago
Created attachment 8480324 [details] [diff] [review]
wip

Addressed the comments and added ICIn_Dense.
Attachment #8478159 - Attachment is obsolete: true
Attachment #8480324 - Flags: review?(kvijayan)
Comment on attachment 8480324 [details] [diff] [review]
wip

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

So, a couple of notes on the ICIn_Dense implementation you have: it doesn't actually check dense arrays.  Typed arrays and regular arrays are not the same, and code that is modern enough to use typed arrays generally doesn't use the "index in array" mechanism to check bounds.  Really you want the ICIn_Array to check for Array objects, not store the index (key) in the stub at all, and write the stub code to do the shape check (which you already do), bounds check on the |initializedLength| of the array, followed by a hole check, followed by returning true.

Also, in the code where you create the stub, make sure to that you're checking for "obj->is<ArrayObject>()" as opposed to "obj->is<TypedArrayObject>", as well as ensuring that the key is within the |initializedLength| bounds, and that it's not a hole, before adding the stub.
Attachment #8480324 - Flags: review?(kvijayan)
(Assignee)

Comment 26

3 years ago
I am a bit confused. The key I stored is not "index of array", but an integer to check whether it is in an array, is my understanding correct? I thought it's like the stubs earlier, just now foo is an integer so I still have to remember foo and obj's shape:

  foo in obj

If I don't remember foo, how should I know whether foo is in obj or not by checking shape, bounds, and hole? Probably I misunderstood dense array, I will try tracing array.
Flags: needinfo?(kvijayan)
(Assignee)

Comment 27

3 years ago
I misunderstood dense array, dense array seems has index the same with the value, so obj[foo] == foo, which there's no need to remember foo.
Flags: needinfo?(kvijayan)
(Assignee)

Comment 28

3 years ago
After read the comments in ObjectImpl.h, I am not sure is comment 27 correct :(
(In reply to Ting-Yu Chou [:ting] from comment #26)
> I am a bit confused. The key I stored is not "index of array", but an
> integer to check whether it is in an array, is my understanding correct? I
> thought it's like the stubs earlier, just now foo is an integer so I still
> have to remember foo and obj's shape:
> 
>   foo in obj
> 
> If I don't remember foo, how should I know whether foo is in obj or not by
> checking shape, bounds, and hole? Probably I misunderstood dense array, I
> will try tracing array.

The runtime treats int32-named properties differently from regular named properties when it comes to dense-element objects, in particular arrays.

So when you do |array.pop()|, for example, the size of the dense-element-array will change, but the array shape won't change at all.  With property names, we guarantee when we attach stubs that the kind of object we are operating on will definitely change shape if the property-names change in some way.  With int32 indexes, we can't guarantee that.

Storing the index in the stub doesn't help because the array could shrink in size without changing shape.  So the shape guard provides no bounds information.  So when attaching stubs for getting elements out of arrays, where the elements are keyed using int32 "properties", we have to do bounds checks.

I would advise taking a look at the implementation of ICGetElem_Dense operation, because your ICIn_Dense will basically mirror that closely (except instead of actually retrieving the element, you'll just return true).
(Assignee)

Comment 30

3 years ago
Created attachment 8482069 [details] [diff] [review]
wip
(Assignee)

Comment 31

3 years ago
Comment on attachment 8482069 [details] [diff] [review]
wip

Updated ICIn_Dense, reference to ICGetElem_Dense.
Attachment #8482069 - Flags: review?(kvijayan)
(Assignee)

Updated

3 years ago
Attachment #8480324 - Attachment is obsolete: true
(Assignee)

Comment 32

3 years ago
Created attachment 8482107 [details] [diff] [review]
wip

Fixed ICIn_Fallback::Compiler::generateStubCode() to push baseline frame, and ICIn_Dense::Compiler::generateStubCode() to load obj->elements before getting initialized length.
Attachment #8482069 - Attachment is obsolete: true
Attachment #8482069 - Flags: review?(kvijayan)
Attachment #8482107 - Flags: review?(kvijayan)
(Assignee)

Comment 33

3 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=dde1be4a4129
Comment on attachment 8482107 [details] [diff] [review]
wip

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

Nice!  This looks pretty good.  I'd like it to get reviewed by another JS team member.  Given the length of time I've looked at this code while in-development, there's likely some degree of snow-blindness.  A fresh set of eyes is good.
Attachment #8482107 - Flags: review?(kvijayan) → review+

Updated

3 years ago
Attachment #8482107 - Flags: review+ → review?(hv1989)
Comment on attachment 8482107 [details] [diff] [review]
wip

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

Thanks, looks fine. I got some high level ideas for some improvements. I didn't look too closely to the generated asm code yet, though.

::: js/src/jit/BaselineIC.cpp
@@ +5685,5 @@
> +        return false;
> +
> +    if (IsCacheableGetPropReadSlot(obj, holder, shape)) {
> +        ICStub::Kind kind = (obj == holder) ? ICStub::In_Native
> +                                            : ICStub::In_NativePrototype;

Nice that we can reuse the code for direct/prototype lookup :D

@@ +5721,5 @@
> +    if (protoChainDepth > ICIn_NativeDoesNotExist::MAX_PROTO_CHAIN_DEPTH)
> +        return true;
> +
> +    RootedShape objShape(cx, obj->lastProperty());
> +    RootedShape lastShape(cx, lastProto->lastProperty());

Those variables aren't used? That seems dodgy?

@@ +5755,5 @@
> +
> +    if (obj->isNative()) {
> +        RootedScript script(cx, frame->script());
> +        if (cond) {
> +            if (key.isInt32() && key.toInt32() >= 0 && !obj->is<TypedArrayObject>()) {

Maybe have a function "ElementAccessIsDenseNative" which does this check. Since this check is used multiple times in this file.

@@ +5764,5 @@
> +                    return false;
> +
> +                stub->addNewStub(denseStub);
> +                return true;
> +            }

I think it would make sense to put this in TryAttachDenseInStub?

@@ +5795,5 @@
>      // Push arguments.
>      masm.pushValue(R1);
>      masm.pushValue(R0);
>      masm.push(BaselineStubReg);
> +    masm.pushBaselineFramePtr(BaselineFrameReg, R0.scratchReg());

Question for djvj. What is this again for? And why do we need it now but before this patch not?
Attachment #8482107 - Flags: review?(hv1989)
(Assignee)

Comment 36

3 years ago
Created attachment 8483944 [details] [diff] [review]
patch

(In reply to Hannes Verschore [:h4writer] from comment #35)
> Comment on attachment 8482107 [details] [diff] [review]
> wip
> 
> Review of attachment 8482107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, looks fine. I got some high level ideas for some improvements. I
> didn't look too closely to the generated asm code yet, though.
> 
> ::: js/src/jit/BaselineIC.cpp
> @@ +5685,5 @@
> > +        return false;
> > +
> > +    if (IsCacheableGetPropReadSlot(obj, holder, shape)) {
> > +        ICStub::Kind kind = (obj == holder) ? ICStub::In_Native
> > +                                            : ICStub::In_NativePrototype;
> 
> Nice that we can reuse the code for direct/prototype lookup :D

Copy from TryAttachNativeGetPropStub() :p

> 
> @@ +5721,5 @@
> > +    if (protoChainDepth > ICIn_NativeDoesNotExist::MAX_PROTO_CHAIN_DEPTH)
> > +        return true;
> > +
> > +    RootedShape objShape(cx, obj->lastProperty());
> > +    RootedShape lastShape(cx, lastProto->lastProperty());
> 
> Those variables aren't used? That seems dodgy?

Removed, also the ones in TryAttachNativeGetPropDoesNotExistStub().

> 
> @@ +5755,5 @@
> > +
> > +    if (obj->isNative()) {
> > +        RootedScript script(cx, frame->script());
> > +        if (cond) {
> > +            if (key.isInt32() && key.toInt32() >= 0 && !obj->is<TypedArrayObject>()) {
> 
> Maybe have a function "ElementAccessIsDenseNative" which does this check.
> Since this check is used multiple times in this file.

Added IsDenseElementAccess() instead, since native checking is separated also in TryAttachGetElemStub().

> 
> @@ +5764,5 @@
> > +                    return false;
> > +
> > +                stub->addNewStub(denseStub);
> > +                return true;
> > +            }
> 
> I think it would make sense to put this in TryAttachDenseInStub?

Added.

> 
> @@ +5795,5 @@
> >      // Push arguments.
> >      masm.pushValue(R1);
> >      masm.pushValue(R0);
> >      masm.push(BaselineStubReg);
> > +    masm.pushBaselineFramePtr(BaselineFrameReg, R0.scratchReg());
> 
> Question for djvj. What is this again for? And why do we need it now but
> before this patch not?

I added that to get the script for compiler.getStubSpace() when newing a stub.
Attachment #8482107 - Attachment is obsolete: true
Attachment #8483944 - Flags: review?(hv1989)
Hannes, pinging on review.  It'd be nice to get this in soon as it blocks Ion optimizations of the same ops.
At a skim, this isn't handling |in| on typed arrays at all.  Am I misreading, or is that something that could/is being punted for now?
Comment on attachment 8483944 [details] [diff] [review]
patch

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

Looks good. Thanks!

::: js/src/jit/BaselineIC.cpp
@@ +3925,5 @@
>             type == Scalar::Float64;
>  }
>  
>  static bool
> +IsDenseElementAccess(HandleValue key, HandleObject obj)

Can you switch the arguments. First obj and afterwards key?
Also can you move the obj->native() check in here too?

::: js/src/jit/MIRGraph.cpp
@@ +893,5 @@
>          clearEntryResumePoint();
>  
>  #ifdef DEBUG
>      if (!entryResumePoint()) {
> +//        MOZ_ASSERT(resumePointsEmpty());

I assume this shouldn't be part of this patch?

::: js/src/vm/Interpreter.cpp
@@ +481,2 @@
>          return CallJSNative(cx, fun->native(), args);
> +    }

Can you revert this change too?
Attachment #8483944 - Flags: review?(hv1989) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #38)
> At a skim, this isn't handling |in| on typed arrays at all.  Am I
> misreading, or is that something that could/is being punted for now?

Punted.  Common case for usage of 'in' with indexes is normal arrays.  JS code modern enough to work with typed arrays is also modern enough to generally use length comparisons.  Counterexamples welcome.
(Assignee)

Comment 41

3 years ago
Created attachment 8486924 [details] [diff] [review]
patch

(In reply to Hannes Verschore [:h4writer] from comment #39)
> Comment on attachment 8483944 [details] [diff] [review]
> patch
> 
> Review of attachment 8483944 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Thanks!

Thanks for reviewing.

> 
> ::: js/src/jit/BaselineIC.cpp
> @@ +3925,5 @@
> >             type == Scalar::Float64;
> >  }
> >  
> >  static bool
> > +IsDenseElementAccess(HandleValue key, HandleObject obj)
> 
> Can you switch the arguments. First obj and afterwards key?
> Also can you move the obj->native() check in here too?

Done, I renamed also the function to IsNativeDenseElementAccess().

> 
> ::: js/src/jit/MIRGraph.cpp
> @@ +893,5 @@
> >          clearEntryResumePoint();
> >  
> >  #ifdef DEBUG
> >      if (!entryResumePoint()) {
> > +//        MOZ_ASSERT(resumePointsEmpty());
> 
> I assume this shouldn't be part of this patch?

No, it shouldn't. Reverted.

> 
> ::: js/src/vm/Interpreter.cpp
> @@ +481,2 @@
> >          return CallJSNative(cx, fun->native(), args);
> > +    }
> 
> Can you revert this change too?

Reverted.

Another try: https://tbpl.mozilla.org/?tree=Try&rev=a907032aa329
Attachment #8483944 - Attachment is obsolete: true
(Assignee)

Comment 42

3 years ago
Created attachment 8487813 [details] [diff] [review]
patch

Rebased to the latest trunk code, try: https://tbpl.mozilla.org/?tree=Try&rev=eefdbbbdbc0a.

Kannan, are there anything else I need to do before setting "checkin-needed" if the try is green?
Attachment #8486924 - Attachment is obsolete: true
Flags: needinfo?(kvijayan)
(Assignee)

Comment 43

3 years ago
linux64-br-haz_try_dep is timed out running 'scripts/scripts/spidermonkey_build.py', will check it, though I am not sure why it didn't fail in previous try.
I don't think that's a related issue.  I would push at this point.  We get random reds at various points due to infrastructure issues not anything else.
Flags: needinfo?(kvijayan)
(Assignee)

Comment 45

3 years ago
Created attachment 8487945 [details] [diff] [review]
patch

OK, add reviewer to commit message.
Attachment #8487813 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/74728e99dc47
Keywords: checkin-needed
(In reply to Kannan Vijayan [:djvj] from comment #44)
> I don't think that's a related issue.  I would push at this point.  We get
> random reds at various points due to infrastructure issues not anything else.

That turns out not to be the case. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c2946c97fa7d
(Assignee)

Comment 48

3 years ago
I will see if it relates to bug 1041688.
(Assignee)

Comment 49

3 years ago
Seems not about gecko. The same patch & parent as comment 41, but still failed linux64-br-haz_try_dep (https://tbpl.mozilla.org/?tree=Try&rev=9761527004e7).

Also it is not related the patch of bug 1041688, the test restuls before/after the patch both failed:

https://tbpl.mozilla.org/?tree=Try&rev=479a376a6ef1,
https://tbpl.mozilla.org/?tree=Try&rev=6dc1f20ce8aa

Haven't found documents for running browser rooting analysis test locally.
(Assignee)

Comment 50

3 years ago
With the patch (attachment 8487945 [details] [diff] [review]), I failed linux64-br-haz_try_dep with time out:

  05:25:55     INFO -  build finished (status 0): Thu Sep 11 08:25:55 2014
  05:25:55     INFO -  Exiting run_complete with status 0
  05:25:55     INFO -  Running callgraph to generate callgraph.txt
  05:25:55     INFO -  PATH="${PATH}:/builds/slave/l64-br-haz_try_dep-00000000000/build/sixgill/usr/bin" ANALYZED_OBJDIR='/builds/slave/l64-br-haz_try_dep-00000000000/build/source/obj-analyzed' XDB='/builds/slave/l64-br-haz_try_dep-00000000000/build/sixgill/usr/bin/xdb.so' SOURCE='/builds/slave/l64-br-haz_try_dep-00000000000/build/source' /builds/slave/l64-br-haz_try_dep-00000000000/build/obj-opt-js/dist/bin/js /builds/slave/l64-br-haz_try_dep-00000000000/build/source/js/src/devtools/rootAnalysis/computeCallgraph.js > callgraph.txt
  05:26:11     INFO -  Finished loading data structures

  command timed out: 3600 seconds without output running ['scripts/scripts/spidermonkey_build.py', '--config-file', 'hazards/build_browser.py', '--config-file', 'hazards/common.py'], attempting to kill
  process killed by signal 9
  program finished with exit code -1
  elapsedTime=11978.513828

Full log here: https://tbpl.mozilla.org/php/getParsedLog.php?id=47876639&tree=Try&full=1#error0

I have read https://wiki.mozilla.org/Javascript:SpiderMonkey:ExactStackRooting, but it's not clear how to run the analysis locally. Could you please teach me that? Thanks.
Flags: needinfo?(sfink)
That is a really weird failure.  I have no idea why this patch is introducing it.  I'll take a quick look through the source and see if I can spot anything off.
(Assignee)

Comment 52

3 years ago
NI jonco also for how to run hazard build locally (comment 50). Please let me know if I should talk to someone else, thanks.
Flags: needinfo?(jcoppeard)
(In reply to Ting-Yu Chou [:ting] from comment #52)

Hi Ting, unfortunately I don't know how to get the hazard analysis running locally for browser builds.  sfink really is person who knows most about this, although I don't think even he has step-by-step instructions to make this work.

I had a look at your change too but I can't see why it would cause the analysis to time out.  However it can be fragile so maybe it got confused by something.
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 54

3 years ago
Created attachment 8517157 [details] [diff] [review]
patch

Rebased to the latest trunk, linux64-br-haz_try_dep is still failed.
(Assignee)

Updated

3 years ago
Attachment #8487945 - Attachment is obsolete: true
(Assignee)

Comment 55

3 years ago
Kannan, do you think we will have a chance to talk to :sfink about the failure when we're in Portland?
Flags: needinfo?(kvijayan)
(Assignee)

Comment 56

3 years ago
I just talked to :sfink, he'll try to help if I can send him a Try number, I'll do this later when I have time.
Flags: needinfo?(kvijayan)
(Assignee)

Comment 57

3 years ago
Created attachment 8531566 [details] [diff] [review]
patch

Rebased the patch, and Try: https://tbpl.mozilla.org/?tree=Try&rev=c87573265f4b
Attachment #8517157 - Attachment is obsolete: true
It's timing out because the analysis is consuming gobs of memory during the compile (5GB at the moment, 3.5GB resident), which is causing then build machine to swap like crazy. I'm not sure why your patch pushed it over the edge, but it's been very close recently so I don't think the patch probably matters all that much.

I'll ni? bhackett here to ask if there's some easy way to prune down xmanager's memory usage. Also, I'll try to ask him in person tomorrow.
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 59

3 years ago
Great, thank you for your help, Steve.
(In reply to Steve Fink [:sfink, :s:] (PTO til Dec 1) from comment #58)
> It's timing out because the analysis is consuming gobs of memory during the
> compile (5GB at the moment, 3.5GB resident), which is causing then build
> machine to swap like crazy. I'm not sure why your patch pushed it over the
> edge, but it's been very close recently so I don't think the patch probably
> matters all that much.
> 
> I'll ni? bhackett here to ask if there's some easy way to prune down
> xmanager's memory usage. Also, I'll try to ask him in person tomorrow.

A few things:

- What has memory usage for xmanager been over time?  When you first started with whole browser builds, do you remember what it was like?

- sixgill tries to keep track of the allocations it performs and you can call PrintAllocs() to get this data.  You could try modifying handle_event in xmanager.cpp to, say, call this function once a minute and see if it indicates where the high memory usage is coming from.

- There should be a -memory-limit option which you can try using.  If we get to high VM usage then xmanager should flush out the entries it keeps in memory for callgraph edges, though I don't remember whether xmanager is even populating these databases.  After the frontend runs, are there body_caller/callee.xdb databases?  What databases and other files do exist after the frontend runs, and how large are they typically?

- I don't think I've tried this before, but the frontend should be able to recover from an interrupted build, if things are shut down cleanly.  When memory usage gets high, you could try killing make, killing xmanager with SIGINT (which tells it to shut down cleanly) and then restarting the build.
Flags: needinfo?(bhackett1024)
The memory usage should be fixed, finally, by bug 1137017.
Flags: needinfo?(sfink)
(Assignee)

Comment 62

3 years ago
Created attachment 8586690 [details] [diff] [review]
patch v4

Rebased. Ask for another review because the code is different since the patch was created.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58dc3ad41eab
Attachment #8531566 - Attachment is obsolete: true
Attachment #8586690 - Flags: review?(kvijayan)
Comment on attachment 8586690 [details] [diff] [review]
patch v4

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

r=me with comment addressed or explained.

::: js/src/jit/BaselineIC.cpp
@@ +6067,5 @@
> +
> +    // Unbox key and bounds check.
> +    Address initLength(scratch, ObjectElements::offsetOfInitializedLength());
> +    Register key = masm.extractInt32(R0, ExtractTemp0);
> +    masm.branch32(Assembler::BelowOrEqual, initLength, key, &failure);

Do we not need to ensure that key >= 0 here?
Attachment #8586690 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 64

3 years ago
(In reply to Kannan Vijayan [:djvj] from comment #63)
> Comment on attachment 8586690 [details] [diff] [review]
> patch v4
> 
> Review of attachment 8586690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed or explained.
> 
> ::: js/src/jit/BaselineIC.cpp
> @@ +6067,5 @@
> > +
> > +    // Unbox key and bounds check.
> > +    Address initLength(scratch, ObjectElements::offsetOfInitializedLength());
> > +    Register key = masm.extractInt32(R0, ExtractTemp0);
> > +    masm.branch32(Assembler::BelowOrEqual, initLength, key, &failure);
> 
> Do we not need to ensure that key >= 0 here?

Refer to ICGetElem_Dense::Compiler::generateStubCode() [1], it also doesn't check key >= 0. I guess

  masm.branchTestMagic(Assembler::Equal, element, &failure);

does make sure it is an element?

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/BaselineIC.cpp#4514
Flags: needinfo?(kvijayan)
(In reply to Ting-Yu Chou [:ting] from comment #64)
> (In reply to Kannan Vijayan [:djvj] from comment #63)
> > Comment on attachment 8586690 [details] [diff] [review]
> > patch v4
> > 
> > Review of attachment 8586690 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with comment addressed or explained.
> > 
> > ::: js/src/jit/BaselineIC.cpp
> > @@ +6067,5 @@
> > > +
> > > +    // Unbox key and bounds check.
> > > +    Address initLength(scratch, ObjectElements::offsetOfInitializedLength());
> > > +    Register key = masm.extractInt32(R0, ExtractTemp0);
> > > +    masm.branch32(Assembler::BelowOrEqual, initLength, key, &failure);
> > 
> > Do we not need to ensure that key >= 0 here?
> 
> Refer to ICGetElem_Dense::Compiler::generateStubCode() [1], it also doesn't
> check key >= 0. I guess
> 
>   masm.branchTestMagic(Assembler::Equal, element, &failure);
> 
> does make sure it is an element?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/js/src/jit/BaselineIC.cpp#4514

Actually, yeah.. the compare here uses an unsigned compare, so any negative values will come out as too-high, exceeding the bounds.  I forget this periodically and it then proceeds to alarm me unnecessarily until I remember it again.  You should be fine.
Flags: needinfo?(kvijayan)
(Assignee)

Comment 66

3 years ago
Created attachment 8590634 [details] [diff] [review]
patch v5

Rebased, again...

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0a56bcb214d
Attachment #8586690 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3da454330a5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f3da454330a5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Nice!

AWFY detected a regression/improvement on:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: no asmjs

Regression(s)/Improvement(s):
- asmjs-ubench: fannkuch: -3.37% (improvement)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7de210d3a493&tochange=40bc31482401

More details: http://arewefastyet.com/regressions/#/regression/236259
Depends on: 1154080
(Assignee)

Comment 70

3 years ago
This causes regression like bug 1154080, which makes Nightly unusable. I guess we want to back this out at first?
(Assignee)

Comment 71

3 years ago
(In reply to Ting-Yu Chou [:ting] from comment #70)
> This causes regression like bug 1154080, which makes Nightly unusable. I
> guess we want to back this out at first?

It seems the problem is there before landing. I'll double check.
(Assignee)

Comment 72

3 years ago
(In reply to Ting-Yu Chou [:ting] from comment #71)
> It seems the problem is there before landing. I'll double check.

Unfortunatelly, it is the patch here causing regression. And I've already found a problem [1] which it should branch to failurePopR0Scratch, though it does not solve bug 1154080.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/BaselineIC.cpp#6123
(Assignee)

Comment 73

3 years ago
When it hangs, I see a lot of spew of this:

  [BaselineIC]   Generating In_NativeDoesNotExist stub

with stack:

  (gdb) bt
  #0  js::jit::ICInNativeDoesNotExistCompiler::getStub (
      this=this@entry=0x7fffffff3750, space=0x7fffdddff940)
      at /home/ting/w/fx/mc/js/src/jit/BaselineIC.cpp:6057
  #1  0x00007ffff303d436 in TryAttachNativeInDoesNotExistStub (
      attached=<synthetic pointer>, obj=..., key=..., stub=0x7fffbddfe520, 
      script=..., cx=<optimized out>)
      at /home/ting/w/fx/mc/js/src/jit/BaselineIC.cpp:5937
  #2  js::jit::DoInFallback (cx=<optimized out>, frame=<optimized out>, 
      stub=0x7fffbddfe520, key=..., objValue=..., res=...)
      at /home/ting/w/fx/mc/js/src/jit/BaselineIC.cpp:5977
  #3  0x00007ffff7fe175b in ?? ()
  #4  0xfffaffffbf6ab970 in ?? ()
  #5  0x00007fffffff3820 in ?? ()
  #6  0xfff9800000000000 in ?? ()
  #7  0x00007ffff5876940 in js::jit::DoGetNameFallbackInfo ()
     from /home/ting/w/fx/mc/obj-x86_64-unknown-linux-gnu/dist/bin/libxul.so
  #8  0x00007fffddf51be0 in ?? ()
  #9  0x00007fffc94395f4 in ?? ()
  #10 0x0000000000000781 in ?? ()
  #11 0x00007fffffff3878 in ?? ()
  #12 0x00007fffbddfe520 in ?? ()
  #13 0xfffaffffbf6ab970 in ?? ()
  #14 0xfffc7fffbbd09ec0 in ?? ()
  ---Type <return> to continue, or q <return> to quit---
  #15 0xfffc7fffbbd09ec0 in ?? ()
  #16 0xfffaffffbf6ab970 in ?? ()
  #17 0xfffc7fffbe4cdb80 in ?? ()
  #18 0x00007fffe757c800 in ?? ()
  #19 0x0000000000000058 in ?? ()
  #20 0x00007fffbda9b880 in ?? ()
  #21 0x00007fffc942b1cc in ?? ()
  #22 0x0000000000000302 in ?? ()
  #23 0x0000000000000001 in ?? ()
  #24 0x0000000400000000 in ?? ()
  #25 0x00007fffffff3948 in ?? ()
  #26 0x00007fffc942b2ae in ?? ()
  #27 0x0000000000000701 in ?? ()
  #28 0x00007fffd9471200 in ?? ()
  #29 0x0000000000000000 in ?? ()
(Assignee)

Comment 74

3 years ago
DumpJSStack() points to https://hg.adblockplus.org/adblockplus/file/1669f6fb7b7e/lib/filterStorage.js#l494.
backed out due to addblock hangs:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c35e6083ede
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 76

3 years ago
(In reply to Ting-Yu Chou [:ting] from comment #74)
> DumpJSStack() points to
> https://hg.adblockplus.org/adblockplus/file/1669f6fb7b7e/lib/filterStorage.
> js#l494.

Probably I should avoid attaching NativeInDoesNotExistStub for this kind of loop, but not sure how?
Flags: needinfo?(kvijayan)
Actually, the fix for this is quite simple.  We're just generating too many optimized stubs, and none of them are actually being used.

The common technique for dealing with this is just to set a limit on the number of stubs generated.  For example, see https://dxr.mozilla.org/mozilla-central/source/js/src/jit/BaselineIC.cpp#8888 :

    if (stub->numOptimizedStubs() >= ICSetProp_Fallback::MAX_OPTIMIZED_STUBS) {
        // TODO: Discard all stubs in this IC and replace with generic setprop stub.
        return true;
    }

Just define a constant MAX_OPTIMIZED_STUBS on ICIn_Fallback (8 should be good), and add the check above.  If a site generates too many optimized stubs, it just stops after the 8th one.
Flags: needinfo?(kvijayan)
Merge of the backout:
https://hg.mozilla.org/mozilla-central/rev/2c35e6083ede

New Firefox nightlies have been triggered.
status-firefox40: fixed → ---
Target Milestone: mozilla40 → ---
(Assignee)

Comment 79

3 years ago
Created attachment 8592605 [details] [diff] [review]
patch v6

Addressed comment 72 and 77, confirmed bug 1154080 is not reproduced.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d01ce178f4e2
Attachment #8590634 - Attachment is obsolete: true
Attachment #8592605 - Flags: review?(kvijayan)
(Assignee)

Comment 80

3 years ago
Comment on attachment 8592605 [details] [diff] [review]
patch v6

It fails Ubuntu ASAN x64 opt jittest-2 consistently, will ask for review later :(
Attachment #8592605 - Flags: review?(kvijayan)
(Assignee)

Comment 81

3 years ago
  if (stub->numOptimizedStubs() >= ICIn_Fallback::MAX_OPTIMIZED_STUBS)
      return true;

Kanna, it's this checking causes ASAN jittest-2 failed, but I don't know how to debug this. I noticed some others use DebugModeOSRvolatileStub, could it be the reason?
Flags: needinfo?(kvijayan)
Hmm.  All the failures are use-after-frees when we're doing eager compilation.  I wonder if this is an artifact.  Have you tried rebasing to a more recent green trunk revision (something with a green treeherder run), and re-trying on top of that?

I don't know off the top of my head what is going on here.  It's a weird error.
Flags: needinfo?(kvijayan)
Tracking this for Firefox 40 since this caused a regression.
tracking-firefox40: --- → +
(In reply to Liz Henry (:lizzard) from comment #83)
> Tracking this for Firefox 40 since this caused a regression.

IIRC this patch is backed out (due to the adblock hangs).
So I don't think there is need for tracking this?
(Assignee)

Comment 85

3 years ago
(In reply to Kannan Vijayan [:djvj] from comment #82)
> Hmm.  All the failures are use-after-frees when we're doing eager
> compilation.  I wonder if this is an artifact.  Have you tried rebasing to a
> more recent green trunk revision (something with a green treeherder run),
> and re-trying on top of that?

Tried, no luck :(

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e7e7aa0da29

It was rebased to this:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c75413a564b2
(Assignee)

Comment 86

3 years ago
There's a for...in loop in Frame-onStep-06.js [1], does this also go to DoInFallback?

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/debug/Frame-onStep-06.js#55
Flags: needinfo?(kvijayan)
(Assignee)

Comment 87

3 years ago
I built debug js shell and executed Frame-onStep-06:

  ting@sweet:~/w/fx/mc/js/src/jit-test$ ./jit_test.py -g --args "--ion-eager --ion-offthread-compile=off" ../build_DBG.OBJ/js/src/shell/js Frame-onStep-06

DoInFallback() is entered 8 times, and the last time is from:

  Fallback hit for (/home/ting/w/fx/mc/js/src/jit-test/tests/debug/Frame-onStep-06.js line 20 > eval:1) (pc=10,line=1,uses=1,stubs=0): In

At this time, stub->numOptimizedStubs_ is altered to 0xcdcdcdcd with following stack, then the MAX_OPTIMIZED_STUBS checking access the invalid address:

  Hardware watchpoint 4: -location stub->numOptimizedStubs_

  Old value = 0
  New value = 3452816845
  memset () at ../sysdeps/x86_64/memset.S:83
  83	../sysdeps/x86_64/memset.S: No such file or directory.
  (gdb) bt
  #0  memset () at ../sysdeps/x86_64/memset.S:83
  #1  0x00000000005ad7db in memset (__len=<optimized out>, __ch=205, 
      __dest=<optimized out>) at /usr/include/x86_64-linux-gnu/bits/string3.h:84
  #2  delete_ (chunk=<optimized out>)
      at /home/ting/w/fx/mc/js/src/ds/LifoAlloc.cpp:43
  #3  js::LifoAlloc::freeAll (this=this@entry=0x7ffff69d77e0)
      at /home/ting/w/fx/mc/js/src/ds/LifoAlloc.cpp:66
  #4  0x00000000008460d4 in ~LifoAlloc (this=0x7ffff69d77e0, 
      __in_chrg=<optimized out>) at /home/ting/w/fx/mc/js/src/ds/LifoAlloc.h:244
  #5  ~ICStubSpace (this=0x7ffff69d77e0, __in_chrg=<optimized out>)
      at /home/ting/w/fx/mc/js/src/jit/JitCompartment.h:70
  #6  ~FallbackICStubSpace (this=0x7ffff69d77e0, __in_chrg=<optimized out>)
      at /home/ting/w/fx/mc/js/src/jit/JitCompartment.h:109
  #7  ~BaselineScript (this=0x7ffff69d77d0, __in_chrg=<optimized out>)
      at /home/ting/w/fx/mc/js/src/jit/BaselineJIT.h:110
  #8  delete_<js::jit::BaselineScript> (this=0x7ffff6945810, p=0x7ffff69d77d0)
      at /home/ting/w/fx/mc/js/src/vm/Runtime.h:390
  #9  js::jit::BaselineScript::Destroy (fop=0x7ffff6945810, 
      script=0x7ffff69d77d0) at /home/ting/w/fx/mc/js/src/jit/BaselineJIT.cpp:457
  #10 0x0000000000856a76 in js::jit::RecompileOnStackBaselineScriptsForDebugMode
      (cx=cx@entry=0x7ffff691b4e0, obs=..., 
      observing=observing@entry=js::Debugger::Observing)
      at /home/ting/w/fx/mc/js/src/jit/BaselineDebugModeOSR.cpp:898
  ---Type <return> to continue, or q <return> to quit---
  #11 0x0000000000614c0e in js::Debugger::updateExecutionObservabilityOfFrames (
      cx=cx@entry=0x7ffff691b4e0, obs=..., observing=js::Debugger::Observing)
      at /home/ting/w/fx/mc/js/src/vm/Debugger.cpp:1942
  #12 0x0000000000614e21 in js::Debugger::ensureExecutionObservabilityOfFrame (
      cx=cx@entry=0x7ffff691b4e0, frame=...)
      at /home/ting/w/fx/mc/js/src/vm/Debugger.cpp:2119
  #13 0x0000000000630ca2 in js::Debugger::getScriptFrameWithIter (
      this=this@entry=0x7ffff6980800, cx=cx@entry=0x7ffff691b4e0, frame=..., 
      maybeIter=maybeIter@entry=0x7fffffff6688, vp=..., vp@entry=...)
      at /home/ting/w/fx/mc/js/src/vm/Debugger.cpp:475
  #14 0x0000000000631265 in getScriptFrame (vp=..., iter=..., cx=0x7ffff691b4e0, 
      this=0x7ffff6980800) at /home/ting/w/fx/mc/js/src/vm/Debugger.h:777
  #15 DebuggerFrame_getOlder (cx=0x7ffff691b4e0, argc=<optimized out>, 
      vp=<optimized out>) at /home/ting/w/fx/mc/js/src/vm/Debugger.cpp:5861
  #16 0x00007ffff7e294ff in ?? ()
  #17 0x00007ffff6983000 in ?? ()
  #18 0x00007fffffff6e20 in ?? ()
  #19 0x0000000000000000 in ?? ()
  (gdb) 

I guess that means DebugModeOSRVolatileStub is needed, am I right? How should I use it properly?
(Assignee)

Comment 88

3 years ago
I use DebugModeOSRVolatileStub and check stub.invalid() before getting numOptimizedStubs(). I am not sure if this enough, since the other places using DebugModeOSRVolatileStub have code like this:

  jsbytecode* pc = stub->icEntry()->pc(frame->script());
  JSOp op = JSOp(*pc);
  FallbackICSpew(cx, stub, "GetProp(%s)", js_CodeName[op]);

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3123d44da88e
Those lines are just there for debug spew (when using IONFLAGS=bl-ic-spew to get verbose messages out of the jits).  It seems the try run with your changes are green.

Good catch with the DebugModeOSRVolatileStub change.  That seems to be the right fix.
Flags: needinfo?(kvijayan)
(Assignee)

Comment 90

3 years ago
Created attachment 8594268 [details] [diff] [review]
patch v7

A complete try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=affe08974de2
Attachment #8592605 - Attachment is obsolete: true
Attachment #8594268 - Flags: review?(kvijayan)
Comment on attachment 8594268 [details] [diff] [review]
patch v7

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

Seems the only new changes here are the DebugModeOSRVolatile RAII thing, and the limit on number of new stubs added.  Otherwise same patch?  Looks good.
Attachment #8594268 - Flags: review?(kvijayan) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/24454f99189f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/24454f99189f
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
This updated patch still gives the improvement ;):

AWFY detected a regression/improvement on:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: no asmjs

Regression(s)/Improvement(s):
- asmjs-ubench: fannkuch: -3.27% (improvement)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=006503d10be7&tochange=1d8f373f909d

More details: http://arewefastyet.com/regressions/#/regression/670618
You need to log in before you can comment on or make changes to this bug.