Make nsJSID/IID/CID objects .equals method slightly faster

RESOLVED WONTFIX

Status

()

Core
XPConnect
RESOLVED WONTFIX
11 years ago
11 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
Created attachment 279952 [details] [diff] [review]
Make JSIDs faster, probably, rev. 1

This patch changes the signature of the (noscript, mainly internal) nsIJSID.equals method to take a nsIDRef instead of a nsIJSID... I suspect that this will improve performance in a few cases. There are some additional optimizations that could be made, such as keeping the IID on nsJSIID instead of forwarding requests to nsIInterfaceInfo... or making nsIJSID.id a "shared" property, except I don't think that will work for threadsafety reasons.
Attachment #279952 - Flags: review?(mrbkap)

Updated

11 years ago
Attachment #279952 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

11 years ago
Attachment #279952 - Flags: superreview?(jst)
Attachment #279952 - Flags: approval1.9?
Comment on attachment 279952 [details] [diff] [review]
Make JSIDs faster, probably, rev. 1

This will be approved for landing in M8, during the freeze, only once jst has SR'd.

Updated

11 years ago
Attachment #279952 - Flags: superreview?(jst)
Attachment #279952 - Flags: superreview+
Attachment #279952 - Flags: approval1.9?
Attachment #279952 - Flags: approval1.9+
Comment on attachment 279952 [details] [diff] [review]
Make JSIDs faster, probably, rev. 1

Oops, re-requesting approval as I'm not supposed to do them any more :)
Attachment #279952 - Flags: approval1.9+ → approval1.9?

Updated

11 years ago
Attachment #279952 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 3

11 years ago
First patch landed on trunk. Didn't help much, so I'm going to try a patch to avoid the alloc/release.

Updated

11 years ago
Depends on: 395467
This appears to have cause a regression for at least one extension in bug 395256. Given the closeness to M8 I would suggest we might want to back this out a.s.a.p. unless we have an understanding of what is going on.
I backed out the first patch because of the regression.
mozilla/js/src/xpconnect/idl/xpcjsid.idl 	1.22
mozilla/js/src/xpconnect/src/xpcjsid.cpp 	1.72

Comment 6

11 years ago
Comment on attachment 279952 [details] [diff] [review]
Make JSIDs faster, probably, rev. 1

>Index: js/src/xpconnect/idl/xpcjsid.idl
>===================================================================
> [scriptable, uuid(C86AE131-D101-11d2-9841-006008962422)]
> interface nsIJSID : nsISupports
...
>-    boolean equals(in nsIJSID other);
>+    boolean equals(in nsIDRef other);

Why was the uuid not updated for this change?
(Assignee)

Updated

11 years ago
Attachment #279952 - Flags: approval1.9+
(Assignee)

Comment 7

11 years ago
Going to mark this WONTFIX... it didn't really improve perf when it landed, and I'm not sure this is a critical codepath for anything right now.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.