982317 - Cleopatra needs to merge markers better

Status: RESOLVED WORKSFORME

(Core :: Gecko Profiler, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Starting with the firefox-2014-03-04-03-02-04-mozilla-central nightly, there's been a severe performance regression in the Gecko Profiler.  Building by hand, I've found that the following patch is the trigger:

http://hg.mozilla.org/mozilla-central/rev/b37ed02f9f4b

 Bug 976260 - Register javascript performance events with the profiler, second attempt at pushing. r=jandem
author	Kannan Vijayan <[e-mail]>
	Mon Mar 03 14:36:08 2014 -0500 (at Mon Mar 03 14:36:08 2014 -0500)

This regression (or at least its severity) isn't always plainly visible.  Here's STR that make it so:

1) Load the testcase for bug 959281 (attachment http://people.mozilla.org/~stmichaud/paperjs.org/examples/voronoi/).
2) Move the mouse around over the testcase's "blobs" for 10 seconds.
3) Click the Gecko Profiler in the toolbar, then click Analyze.

In builds with the patch for bug 976260, the browser UI slows down so much as to become unusable.

This is true for both of the most recent versions of the Gecko Profiler -- 1.12.20 and 1.12.21, available at https://github.com/bgirard/Gecko-Profiler-Addon/raw/master/geckoprofiler.xpi.  (See also https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/Existing_Tools.)

I tested on OS X 10.7.5.

Comment 1

[Kannan Vijayan [:djvj]]

Yeah, the code is causing an extremely high number of bailouts which are getting recorded as events in the profiling buffer and the gecko profiler UI is having difficulty rendering them.

Comment 2

[Benoit Girard (:BenWa)]

As far as we know the bailout events are correct. We've never seen so many bailout in a test case so this is likely the root of bug 959281.

What you're seeing is the UI doesn't handle that volume of markers. We have code to merge this but looks like it's not merging the data well enough. Morphing the bug to handle this better in the UI.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> so this is likely the root of bug 959281

Probably not:  The "performance" regression at bug 959281 actually causes lower CPU usage, so it's some kind of event starvation.

But what is a "bailout event", exactly?

Comment 4

[Kannan Vijayan [:djvj]]

Bailout events are occurrences where we are executing in Ion (high-performance jitcode), but some property of the underlying JS execution forces Ion to "bail" to Baseline (low-performance jitcode).  This process is very expensive (it involves manually traversing the optimized Ion frames, unpacking them, reformatting them into baseline frames, writing that out onto the C stack, and then jumping into the baseline jitcode at a point corresponding to where the bailout occurred in the Ion code).

Bailouts are not supposed to occur often.  And if they do, Ion is supposed to mark the function as "not highly optimizable" and just stop trying to execute it in Ion.  After that, the code should only execute in baseline (which can never suffer from bailouts).

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Could a high number of bailout events lead to event starvation?  Or is that problem unrelated?

I suspect the answers are "no" and "yes".

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Also note that the patch for bug 953435, which triggered bug 959281, was backed out a while ago and is no longer present in trunk code.

I suspect the bailouts happen with or without the patch for bug 959281 -- another reason to suspect they're unrelated to bug 959281.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

> I suspect the bailouts happen with or without the patch for bug 959281

I suspect the bailouts happen with or without the patch for bug 953435.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Kannan, this is what you said at bug 959281 comment #92:

Here's where the bailout is happening:

Bailout_Normal at pop on line 12174 of http://people.mozilla.org/~stmichaud/paperjs.org/assets/js/paper.js:12172

Here is the stack at that point:
    js::Shape::replaceLastProperty(js::ExclusiveContext*, js::StackBaseShape&, js::TaggedProto, JS::Handle<js::Shape*>)
    SegmentPoint() @ paper.js:5027
    Segment() @ paper.js:4755
    js::RunScript
    paper</<.statics.read() @ paper.js:319
    paper</Path<.add() @ paper.js:6505
    createPath() @ paper.js line 12313 > eval:76
    renderDiagram() @ paper.js line 12313 > eval:25
    onMouseMove() @ paper.js line 12313 > eval:17
    js::RunScript
    callHandlers() @ paper.js:614
    paper</Callback.fire() @ paper.js:605
    paper</Tool<._fireEvent() @ paper.js:10883
    paper</Tool<._handleEvent() @ paper.js:10906
    handleMouseMove() @ paper.js:10040
    mousemove() @ paper.js:10051
    js::RunScript
    nsEventDispatcher::Dispatch
    event::nsViewManager::DispatchEvent
    Startup::XRE_Main
    (root)

Unfortunately the URL you gave can't be resolved, though the following can:

http://people.mozilla.org/~stmichaud/paperjs.org/assets/js/paper.js

Here's the function at line 12172:

function _$_(left, operator, right) {
	var handler = binaryOperators[operator];
	if (left && left[handler]) {
		var res = left[handler](right);
		return operator === '!=' ? !res : res;
	}
	switch (operator) {
	case '+': return left + right;
	case '-': return left - right;
	case '*': return left * right;
	case '/': return left / right;
	case '%': return left % right;
	case '==': return left == right;
	case '!=': return left != right;
	}
}