refactor the xpt interface info manager a bit

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments)

11.04 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
29.06 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.89 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
6.41 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
15.44 KB, patch
bobbyholley
: review+
benjamin
: review+
Details | Diff | Splinter Review
32.28 KB, patch
benjamin
: review+
mbrubeck
: checkin-
Details | Diff | Splinter Review
1.47 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review
3.08 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
1.28 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
7.18 KB, patch
benjamin
: review-
Details | Diff | Splinter Review
7.20 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
this isn't complete, but I haven't worked on it actively in a while, and what I have seems useful so I just assume it live in the tree instead of my queue.

My main motivation here is getting rid of its usage of nsIEnumerator and nsISupportsArray
Attachment #735413 - Flags: review?(benjamin)
I'm not sure I like this better than swinging the local includes hammer, but its what I did first so I'll let you decide what you think
Attachment #735415 - Flags: review?(benjamin)
feel free to bounce :)
Attachment #735417 - Flags: review?(bobbyholley+bmo)
Attachment #735418 - Flags: review?(bobbyholley+bmo)
Attachment #735420 - Flags: review?(bobbyholley+bmo)
Attachment #735420 - Flags: review?(benjamin)
Attachment #735417 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 735418 [details] [diff] [review]
remove nsXPConnect::mInterfaceInfoManager

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ +248,5 @@
>  
>  nsresult
>  nsXPConnect::GetInfoForName(const char * name, nsIInterfaceInfo** info)
>  {
> +  return XPTInterfaceInfoManager::GetSingleton()->GetInfoForName(name, info);

AFAICT this will change the failure code in the not-found case (NS_ERROR_FAILURE instead of NS_ERROR_NO_INTERFACE), but it almost certainly doesn't matter. :-)
Attachment #735418 - Flags: review?(bobbyholley+bmo) → review+
Attachment #735413 - Flags: review?(benjamin) → review+
Attachment #735415 - Flags: review?(benjamin) → review+
Actually, I suspect that it does matter that we use the same error code: if only because there's a lot of googlejuice for NS_ERROR_NO_INTERFACE explaining that it probably means you're not packaging the right XPT files. I think we should preserve the old behavior.
Comment on attachment 735420 [details] [diff] [review]
remove XPTIInterfaceInfoManager::EnumerateInterfaces()

r=me on the XPCOM bits
Attachment #735420 - Flags: review?(benjamin) → review+
Comment on attachment 735420 [details] [diff] [review]
remove XPTIInterfaceInfoManager::EnumerateInterfaces()

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

r=bholley with comments addressed

::: js/xpconnect/src/XPCComponents.cpp
@@ +251,5 @@
>      switch (enum_op) {
>          case JSENUMERATE_INIT:
>          case JSENUMERATE_INIT_ALL:
>          {
> +            *statep = UINT_TO_JSVAL(0);

maybe JSVAL_ZERO here.

@@ +291,5 @@
>                                         JSObject * *objp, bool *_retval)
>  {
>      JSAutoByteString name;
> +    if (JSID_IS_STRING(id) && name.encodeLatin1(cx, JSID_TO_STRING(id)) &&
> +            name.ptr()[0] != '{') { // we only allow interfaces by name here

Align |name| with JSID_IS_STRING and move the opening brace onto a new line.

@@ +523,5 @@
>      switch (enum_op) {
>          case JSENUMERATE_INIT:
>          case JSENUMERATE_INIT_ALL:
>          {
> +            *statep = UINT_TO_JSVAL(0);

JSVAL_ZERO (same as above)

@@ +528,2 @@
>              if (idp)
> +                *idp = INT_TO_JSID(mInterfaces.Length()); // indicate that we don't know the count

This comment is now wrong.

::: xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp
@@ +273,5 @@
>      nsCOMPtr<nsIInterfaceInfo> ii;
> +    if (NS_SUCCEEDED(EntryToInfo(entry, getter_AddRefs(ii)))) {
> +      bool scriptable = false;
> +      ii->IsScriptable(&scriptable);
> +      if (scriptable) {

There are no other consumers of this stuff that depend on enumerating non-scriptable interfaces, right?
Attachment #735420 - Flags: review?(bobbyholley+bmo) → review+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Actually, I suspect that it does matter that we use the same error code: if
> only because there's a lot of googlejuice for NS_ERROR_NO_INTERFACE
> explaining that it probably means you're not packaging the right XPT files.
> I think we should preserve the old behavior.

fiar enough, it seems like GetInterfaceInfoFor() or whatever it is should just return NS_ERROR_NO_INTERFACE, but for simplicity / not risking breaking stuff I'll just turn other failures GetInterfaceInfoFor() returns into NS_ERROR_NO_INTERFACE unless someone objects
add XPTInterfaceInfoManager.h this time (other than that file its the same patch)
Attachment #736579 - Flags: review?(benjamin)
Comment on attachment 736579 [details] [diff] [review]
export a header declaring XPTInterfaceInfoManager

XPTInterfaceInfoManager.h needs a license header and an emacs modeline. You're using 4-space indent (I guess to be consistent with the rest of the directory), so please make sure the modeline does that also.
Attachment #736579 - Flags: review?(benjamin) → review+
feel free to toss to bhooley, but I figure its pretty obvious and you're around.
Attachment #737815 - Flags: review?(bzbarsky)
Comment on attachment 737815 [details] [diff] [review]
bug 860027 - fix tracemalloc max heap regression

What reinitializes mInterfaces the _next_ time this is enumerated?
(In reply to Boris Zbarsky (:bz) from comment #15)
> Comment on attachment 737815 [details] [diff] [review]
> bug 860027 - fix tracemalloc max heap regression
> 
> What reinitializes mInterfaces the _next_ time this is enumerated?

"nothing"? I wasn't aware that could happen.  new patch coming.
Might be worth adding some tests....
Comment on attachment 737815 [details] [diff] [review]
bug 860027 - fix tracemalloc max heap regression

Might be worth filing a new bug on this....
Attachment #737815 - Flags: review?(bzbarsky) → review-
It looks like this or bug 860572 also caused some Ts Paint regressions on Windows, and probably on Linux too.  Please consider backing this out; otherwise file a follow-up bug.
Comment on attachment 736579 [details] [diff] [review]
export a header declaring XPTInterfaceInfoManager

(In reply to Matt Brubeck (:mbrubeck) from comment #20)
> It looks like this or bug 860572 also caused some Ts Paint regressions on
> Windows, and probably on Linux too.  Please consider backing this out;
> otherwise file a follow-up bug.

With tbsaunde's permission via dev-tree-management, I backed out just this patch on inbound, to test whether that has any effect on the Ts or Trace Mallocs regressions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/571cbd297f5a

If that doesn't fix the regressions, we can back out the rest of this bug and/or bug 860572.  If this bug turns out not to be the culprit, it can re-land.
Attachment #736579 - Flags: checkin-
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
as best I could tell setting the size of the array before appending stuff to it in GetScriptableInterfaces() fixes the talos regressions, and it seems trivial enough I relanded part 5
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e44c2d6d72
https://hg.mozilla.org/mozilla-central/rev/e7e44c2d6d72
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
It looks like this caused similar Talos regressions, at least on some platforms.  Unfortunately they were not easy to detect in the Try push because the regression was within the noise level of this test on Ubuntu 64, but it is clear in the results from XP which seems to be less noisy:

Try push with updated part 5:
https://tbpl.mozilla.org/?tree=Try&rev=bc087c3ed58a

compare-talos link for that Try push:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=d8db39f57349&newRev=bc087c3ed58a&submit=true

New tree-management thread:
https://groups.google.com/d/topic/mozilla.dev.tree-management/23vVQA3uF_o/discussion

> Regression: Mozilla-Inbound-Non-PGO - Ts Paint, MAX Dirty Profile - XP -
> 5.61% increase
>    Previous: avg 694.180 stddev 3.306 of 12 runs up to revision 2b56dbd338cf
>    New     : avg 733.118 stddev 14.515 of 12 runs since revision e7e44c2d6d72
>    Change  : +38.938 (5.61% / z=11.777)
>    Graph   : http://mzl.la/15qPKp5

Previous tree-management thread for reference:
https://groups.google.com/d/topic/mozilla.dev.tree-management/l-KISrPTlmc/discussion
Backed out part 5 again, sorry:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a441b303a00
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think this will fix the startup time regression, but if not I'm also off to rice the hash table iterator stuff some too.
Attachment #745351 - Flags: review?(bobbyholley+bmo)
Attachment #745351 - Flags: review?(bobbyholley+bmo) → review+
Attachment #746023 - Flags: review?(benjamin) → review+
Comment on attachment 746024 [details] [diff] [review]
bug 860027 - rename xptiInterfaceInfoEntry::GetInterfaceInfo() to InterfaceInfo() since it can't return null and make it return an already_AddRefed

-    return NS_SUCCEEDED(mEntry->Parent()->GetInterfaceInfo(&mParent));
+    // XXX tbsaunde why doesn't this leak? or is it never called?
+    mEntry->Parent()->InterfaceInfo();

I don't remember what parent interfaceinfo actually is. But I'm pretty sure that what we need here is "mParent = mEntry->Parent()->InterfaceInfo()"
Attachment #746024 - Flags: review?(benjamin) → review-
Comment on attachment 749721 [details] [diff] [review]
bug 860027 - rename xptiInterfaceInfoEntry::GetInterfaceInfo() to InterfaceInfo() since it can't return null and make it return an already_AddRefed

r=me with extra XXX comment removed
Attachment #749721 - Flags: review?(benjamin) → review+
You need to log in before you can comment on or make changes to this bug.