Closed Bug 1540301 Opened 6 years ago Closed 6 years ago

Replace instances of nsXPCWrappedJSClass with nsXPTInterfaceInfo

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [overhead:2k])

Attachments

(5 files)

I was looking over nsXPCWrappedJSClass just now, and I see no reason that we need to dynamically allocate these. At a glance, it looks like they are just a thin wrapper around XPT data. Maybe we could also then statically allocate mWrappedJSClassMap. The first step would be to figure out how much memory these actually use.

I hacked up a size of function for XPCJSRuntime::mWrappedJSClassMap and ran it in a browser with TechCrunch and CNN open. The total size for this hash table and the XPCWrappedJSClasses in the various non-main processes was 1936 bytes, 1864 bytes, 1864 bytes, 944 bytes and 568 bytes. In the main process, it was 7960 bytes. Only about a dozen entries in the content process. I was surprised it was so small!

For my own curiosity, these are the interfaces that appear in this table across the various content processes: nsISupports, nsIConsoleAPIStorage, nsITimerCallback, nsIFactory, nsIAsyncShutdownService, nsIObserver, nsIWebProgressListener, mozIExtensionProcessScript, nsIAutoCompletePopup, nsIAsyncShutdownService, nsIAboutNewTabService, nsIPrivacyTransitionObserver, nsIWebBrowserChrome3, nsISHistoryListener.

Whiteboard: [overhead:2k]

Hopefully I'll be able to look at this soon. It is a small win, but it seems fairly self-contained.

Assignee: nobody → continuation
Depends on: 1541684

I have patches for this now. The first thing I do, in bug 1541684, is get rid of all of the fields except for the XPT info. Then I have a series of patches that make all of the methods on nsXPCWrappedJSClass static. Most of the methods already don't use |this| anyways. I also eliminate nsIXPCWrappedJSClass to simplify things a bit, as it seems pointless. After that, replacing the uses of nsXPCWrappedJSClass with nsXPTInterfaceInfo is straightforward. nsXPCWrappedJSClass is left as a collection of static method, and the ctor/dtor/Isupports stuff, as well as IID2WrappedJSClassMap, can be removed.

Arguably all of the nsXPCWrappedJSClass methods should become nsXPCWrappedJS methods, but the code is pretty gnarly, so I wanted to leave it in place so the history is still easily there.

Summary: Statically allocate nsXPCWrappedJSClass → Replace instances of nsXPCWrappedJSClass with nsXPTInterfaceInfo
Type: enhancement → task
Type: task → enhancement

There are a number of nsXPCWrappedJSClass methods that don't use any
data from |this|, so go ahead and make them static. This is one step
towards eliminating nsXPCWrappedJSClass entirely.

In addition, devirtualize a few methods, because they are going to
have to get devirtualized anyways, and there's no need for them to be
virtual.

The first idea here is that |this| is actually the GetClass() of the
|wrapper| argument (the one call site looks like
"GetClass()->CallMethod(this, ...)"), so we can locally reconstruct it
when CallMethod is a static method.

The second idea here is that the only real use of the
nsXPCWrappedJSClass is to grab some data from the nsXPTInterfaceInfo
in a few places. This means that we can take a pointer to the info
early on in the function and use that rather than go through the
nsXPCWrappedJSClass. This in turn means that because the info is
statically allocated we no longer need to do a kungFuDeathGrip on the
wrapper's nsXPCWrappedJSClass.

This interface serves no real purpose, aside from some weird XPConnect
debugging function. If you are in a debugger already, you can just
call the nsXPCWrappedJSClass DebugDump method directly.

For now, I changed nsXPCWrappedJSClass to just implement nsISupports,
but this will go away later.

I also devirtualized DebugDump(), because it is no longer an XPCOM
method.

Ultimately, this method is really about dumping XPConnect-ish
information about an nsXPTInterfaceInfo, so change it into a static
method that takes an info directly. This loses logging of the
nsXPCWrappedJSClass's ref count, but that is going away in the next
patch anyways.

nsXPCWrappedJSClass now only adds indirections and dynamic
allocations, so eliminate it. The class is kept around as a collection
of static helper functions to avoid needing to move around all of this
code and messing with history. In total, these patches save around 2kb
of dynamic allocations per process.

The major parts of this patch are:

  • Dropping indirection for accessing things on nsXPTInterfaceInfo and
    renaming things from class to info.
  • Removing the stuff related to instances of nsXPCWrappedJSClass
    existing (ctor, dtor, field, ISupports implementation, getter methods).
  • Removing IID2WrappedJSClassMap, because we only need the map from IIDs
    to info.

I dropped the null check in TraverseNative because mInfo is never
cleared, while mClass was.

I dropped the forward declaration of nsXPCWrappedJSClass because no
instances of that class exist, so no function will take or return a
pointer to one.

Blocks: 1542024
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e603c009a99b part 1 - Make trivially static nsXPCWrappedJSClass methods static. r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/cdc953c774f4 part 2 - Make nsXPCWrappedJSClass::CallMethod() into a static method. r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/51753d8777fa part 3 - Eliminate nsIXPCWrappedJSClass. r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/7f52f402ed0b part 4 - Make nsXPCWrappedJSClass::DebugDump into an infallible static method. r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/66d2447c66fa part 5 - Replace instances of nsXPCWrappedJSClass with nsXPTInterfaceInfo. r=bzbarsky
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: