Closed Bug 1127987 Opened 5 years ago Closed 5 years ago

5% regression on octane-Box2D on Jan 29th

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 + fixed
firefox37 + fixed
firefox38 + fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: guijoselito, Assigned: bhackett)

References

Details

(Keywords: regression)

Attachments

(1 file)

Flags: needinfo?(bhackett1024)
Blocks: CVE-2015-0820
No longer blocks: 1123874
Flags: needinfo?(bhackett1024)
Attached patch patchSplinter Review
This is actually a regression from bug 1125389.  I transposed the parent and metadata arguments in the getInitialShape call.
Assignee: nobody → bhackett1024
Attachment #8557663 - Flags: review?(jdemooij)
Comment on attachment 8557663 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1125389
[User impact if declined]: performance regression, possible devtools weirdness due to a bug in the patch for the above bug.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: none
Attachment #8557663 - Flags: approval-mozilla-beta?
Attachment #8557663 - Flags: approval-mozilla-aurora?
Brian, please request uplifts when the patch has been at least reviewed
Comment on attachment 8557663 [details] [diff] [review]
patch

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

::: js/src/vm/Shape.cpp
@@ +646,1 @@
>                                                           res->getMetadata(),

Hm the order was correct, AFAICS:

    static Shape *getInitialShape(ExclusiveContext *cx, const Class *clasp,
                                  TaggedProto proto, JSObject *metadata,
                                  JSObject *parent, size_t nfixed, uint32_t objectFlags = 0);
Attachment #8557663 - Flags: review?(jdemooij)
This also affects octane-richards on x86 and x64:

Before: 30268
After: 29760

The first bad revision is:
changeset:   226558:51ac953371fe
user:        Brian Hackett <bhackett1024@gmail.com>
date:        Thu Jan 29 11:50:43 2015 -0700
summary:     Bug 1125389 - Fix NewReshapedObject to use the old shape's data, r=jandem.
Comment on attachment 8557663 [details] [diff] [review]
patch

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

evilpie just noticed that Shape.h does not match the cpp file. Can you update the two getInitialShape definitions in Shape.h as well? r=me with that.
Attachment #8557663 - Flags: review+
(In reply to Jan de Mooij [:jandem] from comment #6)
> evilpie just noticed that Shape.h does not match the cpp file. Can you
> update the two getInitialShape definitions in Shape.h as well? r=me with
> that.

Yeah, that explains it.  I fixed the definitions and also did an audit of the uses and found/fixed two other places the arguments are transposed, though both are in the new unboxed object code.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3714f520752
https://hg.mozilla.org/mozilla-central/rev/c3714f520752
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Thanks. This fixes the mentioned regressions on AWFY.
Attachment #8557663 - Flags: approval-mozilla-beta?
Attachment #8557663 - Flags: approval-mozilla-beta+
Attachment #8557663 - Flags: approval-mozilla-aurora?
Attachment #8557663 - Flags: approval-mozilla-aurora+
Just noticed this introduces a new regression:
http://arewefastyet.com/#machine=28&view=single&suite=octane&subtest=DeltaBlue&start=1422886850&end=1422914220

Locally I'm able to reproduce, but only if running the whole suite:

Before:
h4writer@h4writer-ThinkPad-W530:~/Build/octane2.0$ js run.js
Richards: 30053
DeltaBlue: 40821
^C

After:
h4writer@h4writer-ThinkPad-W530:~/Build/octane2.0$ js run.js
Richards: 30755
DeltaBlue: 38626
^C
(In reply to Hannes Verschore [:h4writer] from comment #10)
> Just noticed this introduces a new regression:
> http://arewefastyet.com/
> #machine=28&view=single&suite=octane&subtest=DeltaBlue&start=1422886850&end=1
> 422914220
> 
> Locally I'm able to reproduce, but only if running the whole suite:
> 
> Before:
> h4writer@h4writer-ThinkPad-W530:~/Build/octane2.0$ js run.js
> Richards: 30053
> DeltaBlue: 40821
> ^C
> 
> After:
> h4writer@h4writer-ThinkPad-W530:~/Build/octane2.0$ js run.js
> Richards: 30755
> DeltaBlue: 38626
> ^C

On second look. This was actually a (fake?) improvement when bug 1125389 landed. So now we are back at the same performance as before bug 1125389. So might be interesting to check why it makes a difference, but I'm not flagging this as a new regression.
Depends on: 1155807
Depends on: 1190272
You need to log in before you can comment on or make changes to this bug.