Closed Bug 1168864 Opened 4 years ago Closed 4 years ago

Use mayResolve hook for addprop stubs

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
Websites/frameworks often add new properties to functions. Currently we can't inline the addprop operation because JSFunction has a resolve hook.

This patch makes us check for the mayResolve hook I added in bug 1155946, so that we can inline property adds on functions and other objects with resolve hooks.

With this patch + the patch in bug 935932, the micro-benchmark below improves from 1190 ms to 30 ms.

function f() {
    for (var i=0; i<1000000; i++) {
	var res = function() {};
	res.x = 2;
	res.y = 3;
	res.z = 4;

    }
    return res;
}
var t = new Date;
f();
print(new Date - t);
Attachment #8611229 - Flags: review?(bhackett1024)
In CheckHasNoSuchProperty, shouldn't that be using curObj instead of obj?
Flags: needinfo?(jdemooij)
Comment on attachment 8611229 [details] [diff] [review]
Patch

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

r+ with a fix for the bug bz found.
Attachment #8611229 - Flags: review?(bhackett1024) → review+
(In reply to Not doing reviews right now from comment #1)
> In CheckHasNoSuchProperty, shouldn't that be using curObj instead of obj?

Good catch, thanks!

Not sure how I missed that.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/db3d40e8b4de
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Could this have caused and do we want to act on that?

A regression/improvement was reported on AWFY:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: Ion

Regression(s)/Improvement(s):
- ss: raytrace: 4% (regression)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b563fdf07f83&tochange=20458c713464

More details: http://arewefastyet.com/regressions/#/regression/1689307
Flags: needinfo?(jdemooij)
Attached patch Followup patchSplinter Review
I added some checks for addProperty hooks, but we actually don't need to check for that one in CheckHasNoSuchProperty because it's only used for x.missingProp or "x in y".

I thought addProperty hooks were uncommon but the ArrayObject class has one.
Flags: needinfo?(jdemooij)
Attachment #8615291 - Flags: review?(bhackett1024)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8615291 - Flags: review?(bhackett1024) → review+
> I thought addProperty hooks were uncommon

Fwiw, most DOM things have them. :(
https://hg.mozilla.org/mozilla-central/rev/306758f7b340
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.