Closed Bug 984464 Opened 10 years ago Closed 10 years ago

cloning app object as mozIApplication warns about "mutating the prototype"

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: myk, Assigned: marco)

Details

Attachments

(1 file, 2 obsolete files)

I recently noticed the following message in Fennec's log:

[JavaScript Warning: "TypeError: mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create" {file: "resource://gre/modules/ " line: 108}]

Ironically, the line in question was added to improve performance over in bug 836605.  But perhaps there's a further performance improvement we can make?  We should see if Object.create (or some other mechanism that avoids mutating the prototype) makes things better.
Yeah, interesting. I remember doing the change because it was noticeably faster. I guess we had js engine chenges since that we hit now.
By the way, this code is quite performance critical so we really need to make sure we're doing whatever the js engine prefers here.
Attached patch Patch (obsolete) — Splinter Review
In my testing this is faster than our current code.
Using Object.create with its second argument (specifying the properties with their values) was slower than our current code.

I've used this code to test:
> for (let i = 0; i < 20000; i++) {
>   appsService.getAppByManifestURL('https://aprivileged.com/manifest.webapp')
> }

This snippet took ~5200 ms with our current code, ~4700 ms with the modified code.
I'd also be interested to know the performance of creating the object with `new mozIApplication()`.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> I'd also be interested to know the performance of creating the object with
> `new mozIApplication()`.

(both with and without moving the property assignments into the mozIApplication constructor)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #5)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> > I'd also be interested to know the performance of creating the object with
> > `new mozIApplication()`.
> 
> (both with and without moving the property assignments into the
> mozIApplication constructor)

Using new mozIApplication() with or without moving the property assignments is almost as fast as Object.create(mozIApplication.prototype). (it's ~4750-4800 ms).
(In reply to Marco Castelluccio [:marco] from comment #6)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #5)
> > (In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> > > I'd also be interested to know the performance of creating the object with
> > > `new mozIApplication()`.
> > 
> > (both with and without moving the property assignments into the
> > mozIApplication constructor)
> 
> Using new mozIApplication() with or without moving the property assignments
> is almost as fast as Object.create(mozIApplication.prototype). (it's
> ~4750-4800 ms).

I think they may be equivalent, it is difficult to say because with this basic testing sometimes |new mozIApplication()| is faster, sometimes |Object.create(mozIApplication.prototype)| is faster.

They're both definitely faster than our current code.
Attached patch Patch (obsolete) — Splinter Review
Removes AppsUtils::cloneAsMozIApplication and converts callers to directly use |new mozIApplication|.
Assignee: nobody → mar.castelluccio
Attachment #8396011 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8408936 - Flags: review?(fabrice)
Comment on attachment 8408936 [details] [diff] [review]
Patch

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

::: dom/apps/src/AppsUtils.jsm
@@ +38,5 @@
>    return Services.io.newURI(aURI, null, foo).prePath != foo.prePath ||
>           Services.io.newURI(aURI, null, bar).prePath != bar.prePath;
>  }
>  
> +function mozIApplication(aApp) {

that won't work on b2g because of the reused global. You need to do:
this.mozIApplication = new function(aApp) {...}
Attachment #8408936 - Flags: review?(fabrice) → review-
Attached patch Patch v2Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=ae3d38b3fe5e
Attachment #8408936 - Attachment is obsolete: true
Attachment #8409414 - Flags: review?(fabrice)
Attachment #8409414 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aaf8037b9e0f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.