790386 - IonMonkey: Regression calling into canvas

Status: RESOLVED DUPLICATE of bug 786126

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

Here's

Comment 1

[Tom S]

I think you are missing something.

Comment 2

[(no longer active)]

Damn you, bugzilla!

See this test case please: http://people.mozilla.org/~jmuizelaar/jsscale/scale.html
Use this image as an example, just save it locally, browse to it, and press "diff" (which should really say "scale"). http://www.seobook.com/images/smallfish.jpg

What this script does is calls an emscripten compiled function and also does some memory copying on a typed array.  The first alert box is the time it took to run the emscriptened code, the second one is how long the total took.  The memory copying bits seem to be an order of magnitude slower in Ion builds.

Comment 3

[Sean Stangl [:sstangl]]

Attachment: iongraph output for diff()

Comment 4

[Sean Stangl [:sstangl]]

`perf top` shows the following breakdown (percentages are based on 8 cores):

> 10.42%  js::ion::IonScript::getOsiIndex(unsigned int) const
>  9.09%  js::ion::OsiIndex::returnPointDisplacement() const
>  8.10%  js::ion::IonFrameIterator::machineState() const
>  7.55%  js::ion::InlineFrameIterator::InlineFrameIterator(js::ion::IonFrameIterator const*)
>  5.18%  js::ion::SnapshotIterator::SnapshotIterator(js::ion::IonFrameIterator const&)
>  5.07%  js::ion::InlineFrameIterator::findNextFrame()
>  4.09%  js::ion::SafepointReader::SafepointReader(js::ion::IonScript*, js::ion::SafepointIndex const*)
>  3.51%  js::ion::IonScript::getSafepointIndex(unsigned int) const
>  2.93%  js::ion::SnapshotReader::SnapshotReader(unsigned char const*, unsigned char const*)
>  2.82%  js::ion::IonFrameIterator::osiIndex() const
>  2.27%  js::ion::GetPcScript(JSContext*, JSScript**, unsigned char**)
>  1.61%  jitcode

These numbers are reproducible, and it doesn't look like we bail excessively.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Sounds like a case where we just need to find what's calling GetPcScript, and make it stop.

Comment 6

[Sean Stangl [:sstangl]]

> (gdb) bt
> #0  js::ion::GetPcScript (cx=0x7fffd9dd29f0, scriptRes=0x7fffffff70d0, pcRes=0x7fffffff7148)
>     at /home/sstangl/dev/ionmonkey/js/src/ion/IonFrames.cpp:633
> #1  0x00007ffff4a0b6a1 in js::ContextStack::currentScript (this=0x7fffd9dd2a90, ppc=0x7fffffff7148)
>     at /home/sstangl/dev/ionmonkey/js/src/vm/Stack-inl.h:533
> #2  0x00007ffff4b07a4c in js_InferFlags (cx=0x7fffd9dd29f0, defaultFlags=0)
>     at /home/sstangl/dev/ionmonkey/js/src/jsobj.cpp:2458
> #3  0x00007ffff4b0dce9 in CallResolveOp (cx=0x7fffd9dd29f0, obj=..., id=..., flags=65535, objp=..., 
>     propp=..., recursedp=0x7fffffff72cf) at /home/sstangl/dev/ionmonkey/js/src/jsobj.cpp:4042
> #4  0x00007ffff4b0e0cd in LookupPropertyWithFlagsInline (cx=0x7fffd9dd29f0, obj=..., id=..., flags=
>     65535, objp=..., propp=...) at /home/sstangl/dev/ionmonkey/js/src/jsobj.cpp:4097
> #5  0x00007ffff4b0e366 in js::baseops::LookupProperty (cx=0x7fffd9dd29f0, obj=..., id=..., objp=..., 
>     propp=...) at /home/sstangl/dev/ionmonkey/js/src/jsobj.cpp:4146
> #6  0x00007ffff49cfaf1 in JSObject::lookupGeneric (cx=0x7fffd9dd29f0, obj=..., id=..., objp=..., propp=
>     ...) at /home/sstangl/dev/ionmonkey/js/src/jsobjinlines.h:992
> #7  0x00007ffff4ac27a2 in JSObject::lookupProperty (cx=0x7fffd9dd29f0, obj=..., name=0x7fffe2125640, 
>     objp=..., propp=...) at /home/sstangl/dev/ionmonkey/js/src/jsobjinlines.h:1000
> #8  0x00007ffff4def699 in TryAttachNativeStub (cx=0x7fffd9dd29f0, cache=..., obj=..., name=..., 
>     isCacheableNative=0x7fffffff756f) at /home/sstangl/dev/ionmonkey/js/src/ion/IonCaches.cpp:275
> #9  0x00007ffff4def8ee in js::ion::GetPropertyCache (cx=0x7fffd9dd29f0, cacheIndex=14, obj=..., vp=...)
>     at /home/sstangl/dev/ionmonkey/js/src/ion/IonCaches.cpp:318

Comment 7

[Sean Stangl [:sstangl]]

The problem is that GetPropertyCache cannot cache the getprop because the object has a getter, specifically nsIDOMHTMLCanvasElement_GetWidth. This will likely be solved by the work in Bug 786126. Leaving open for verification.

Comment 8

[Jan de Mooij [:jandem]]

See bug 791219 comment 0 for another testcase where GetPcScript makes us a *lot* slower than JM+TI. It's under SetPropertyCache this time.

Since GetPcScript can be a huge performance fault, I really think we should investigate other fixes to avoid GetPcScript completely here. Bug 786126 (getter IC) will fix one important use case but there are others and this keeps showing up in profiles...

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

In both of these cases, GetPcScript is uncovering a bug elsewhere in the engine. Removing it entirely would not be enough to fix our performance faults - we'd still be 10X slower in bug 791219. (If anything, it's a useful red light that something else is wrong.)

In particular in this bug, js_InferFlags is an architectural flaw in the engine. Not only is it slow, but it's unsafe (Jason and I have both debugged horrible things where the inferred state doesn't relate to the callstack). The flags should be threaded through the property accessors. But in addition to that we're missing an IC that we needed anyway.

The question is whether there are cases where GetPcScript must be called (either because of a design flaw in the engine that cannot be easily fixed - like type monitoring or js_InferFlags, or something legitimate like stack walking during GC), *and* it is the actual cause of a performance fault.

If that's the case, we will have no choice but to optimize it in the short term. But these two bugs aren't convincing.

Where else have we seen it in profiles?

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Triaging as priority for Fx18, but all that should be required is retesting after the DOM work lands.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Verified fixed.