Replace instances of nsXPCWrappedJSClass with nsXPTInterfaceInfo

RESOLVED FIXED in Firefox 68

Status

()

enhancement
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [overhead:2k])

Attachments

(5 attachments)

Assignee

Description

3 months ago

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.

Assignee

Comment 1

3 months ago

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!

Assignee

Comment 2

3 months ago

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.

Assignee

Updated

3 months ago
Whiteboard: [overhead:2k]
Assignee

Comment 3

3 months ago

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

Assignee: nobody → continuation
Assignee

Updated

3 months ago
Depends on: 1541684
Assignee

Comment 4

3 months ago

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.

Assignee

Comment 5

3 months ago

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.

Assignee

Updated

3 months ago
Summary: Statically allocate nsXPCWrappedJSClass → Replace instances of nsXPCWrappedJSClass with nsXPTInterfaceInfo
Assignee

Updated

3 months ago
Type: enhancement → task
Type: task → enhancement
Assignee

Comment 6

3 months ago

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.

Assignee

Comment 7

3 months ago

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.

Assignee

Comment 8

3 months ago

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.

Assignee

Comment 9

3 months ago

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.

Assignee

Comment 10

3 months ago

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.

Assignee

Updated

3 months ago
Blocks: 1542024

Comment 11

2 months ago
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.