Patch for bug 609212 doesn't entirely fix the "profiling makes us slower" thing

NEW
Unassigned

Status

()

Core
JavaScript Engine
7 years ago
3 years ago

People

(Reporter: bz, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

In-browser times with that patch for the testcase (linked in the url field):

-j: 515ms
-j -m: 520ms
-j -m -p: 640-700ms (very noisy)
-m: 1116ms

For comparison, Chrome 10 dev build is 644ms.  So on my machine we're about at parity with that; on the machine of the original reporter of bug 609212 (which is presumably 32-bit builds, since it's Windows; the above numbers for us are all 64-bit on Mac) we're about 16% slower than V8.  I don't know how much it matters that his machine is also slower than mine and may or may not have a bigger cache, etc.

Comment 1

7 years ago
On the testcase for build: Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110115 Firefox/4.0b10pre ID:20110115102915 I get FPS: Processing: 1950ms

On the testcase for Chrome 8.0.552.237 I get FPS: Processing: 1691ms

~B

Updated

7 years ago
Depends on: 609212

Comment 2

4 years ago
Nightly 930-1100ms
Chrome 31 320-340ms
IE 10 850-910ms

Maybe the problem is 609296.
Blocks: 499198
> Maybe the problem is 609296.

It's possible.

A profile shows that on main thread about 1/3 of the time is under js::CallProperty called from jitcode.  Almost half of this is js::PrimitiveToObject.  Most of the rest is js::baseops::GetProperty, mostly under LookupPropertyWithFlagsInline.  Presumably this is some property being gotten on a primitive string that we don't manage to fast-path.

20% is js::SetObjectElement.  Most of this is JSObject::growElements and its realloc?
10% is js_num_toString.  
7% is js::jit::ArrayPushDense.
7% is str_toLowerCase.
5% is js::ConcatStrings
5% is CloneRegExpObject.
4% is str_replace
2% is js::jit::CheckOverRecursed
2% is js::jit::CharCodeAt

So the CallGetProperty and SetObjectElement are the main obvious issues.
Depends on: 609296
Flags: needinfo?(jdemooij)
(Note that Peacekeeper no longer seems to have this test. Unfortunate because this test is a lot more reasonable than the other JS tests they have...)

I'll take a look though.
The patch in bug 609296 improves this from 636 ms to 427 ms, that should take care of the CallGetProperty I think. Will look into the SetObjectElement calls now...
Flags: needinfo?(jdemooij)
The SetObjectElement calls we can fix by eagerly allocating this array: "new Array(80)". Bug 799122 did that for the most part, except when Array is inlined in Ion :(

That should get us to 325 ms or so, with bug 609296 almost 2x faster than trunk but still slower than d8. 

There's a bunch of smaller issues: Ion could inline number.toString(), with GGC MArrayPush can allocate elements directly without calling ArrayPushDense, etc.
Flags: needinfo?(jdemooij)

Updated

4 years ago
Depends on: 959161
Filed bug 959161 for the first part of comment 6.

Updated

4 years ago
Flags: needinfo?(jdemooij)

Comment 8

4 years ago
Nightly for me now is 480-550ms... a 50% improvement in 2 months!
(Assignee)

Updated

3 years ago
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.