Closed Bug 1444745 Opened 6 years ago Closed 6 years ago

Eliminate xptiInterfaceEntry

Categories

(Core :: XPCOM, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mccr8, Assigned: nika)

References

(Blocks 2 open bugs)

Details

Attachments

(12 files, 7 obsolete files)

18.93 KB, application/x-gzip
Details
21.58 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
11.50 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
64.53 KB, patch
erahm
: review-
erahm
: feedback+
Details | Diff | Splinter Review
55.29 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
135.54 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
62.88 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
1.83 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
4.91 KB, patch
Details | Diff | Splinter Review
7.82 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
3.90 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
11.64 KB, patch
erahm
: review+
Details | Diff | Splinter Review
Once bug 1438688 is fixed, it should be possible to eliminate xptiInterfaceEntry, which is a remaining dynamically allocated XPT class. I think this will eliminate most of the remaining 10kb of dynamic information that is not hash tables. It isn't clear that this is worth the effort.

xptiInterfaceEntry is essentially a wrapper around XPTInterfaceDescriptor that stores a few extra pieces of data. If we can statically compute those pieces of data and add them to XPTInterfaceDescriptor, then we can eliminate xptiInterfaceEntry.

This extra data is:
- mMethodBaseIndex and mConstantBaseIndex count the sum of the number of methods and constants, respectively, in all of the parent interfaces. With bug 1438688, we have the full definition of all parent classes available statically, so this can be computed statically.
- mFlags mostly stores the same data as the flags in XPTInterfaceDescriptor. One of the things it stores that the descriptor does not is notxpcom, which is true if any of the methods of the interface are notxpcom, or the parent is notxpcom. As with the base indexes, once all of the parents are statically available, this can be statically computed. The other extra data this stores is the dynamic resolution state. With xptiInterfaceEntry eliminated, there is no dynamic resolution, so this is not needed.
- mInfo is a weak pointer to a xptiInterfaceInfo, which is a dynamically allocated wrapper (yes, a wrapper wrapper). This ensures that a xptiInterfaceEntry never has more than one wrapper. This can be eliminated either by adding a hash table from XPTInterfaceDescriptor to xptiInterfaceInfo to the xptiTypelibGuts (at the cost of some memory), or by making XPTInterfaceDescriptor somehow implement nsIInterfaceInfo, and eliminating xptiInterfaceInfo entirely.
- mParent is the xptiInterfaceEntry of the parent. If we merge the two classes, this won't be needed, because we can just use the XPTInterfaceDescriptor parent.
- mTypelib is a xptiTypelibGuts* that is used to essentially look up the mapping from XPTInterfaceDescriptor to xptiInterfaceEntry. If the latter is eliminated, this is not needed any more. (Also, xptiTypelibGuts will be a singleton, so it isn't needed as a field anyways.)
Depends on: 1449747
Depends on: 1450321
I spent some time trying to figure out if I could use XPTInterfaceDescriptor in place of nsIInterfaceInfo. I think this will not work easily because we also need to support ShimInterfaceInfo, which dynamically looks up things from WebIDL. In principle, we could statically generate the shim info as XPTInterfaceDescriptor, but bug 1438688 works by having a single global generation step, so I'd have to make XPIDL depends on WebIDL in the build system, which sounds like a lot of trouble.

So, I think the way forward for that is to add a hash table somewhere instead of the weak pointer. We could also generate a new xptiInterfaceInfo every time one is requested (it looks like ShimInterfaceInfo does this), but that might require a lot of extra memory for wrappers.
Assignee: nobody → continuation
Priority: -- → P2
Depends on: 1450359
I have this mostly working, but it sounds like Nika was looking through this code for another reason and also noticed that we don't really need the dynamic allocations, and wrote up the patches to get rid of it. Hers apparently goes a little further than mine and does the perfect hashing stuff to get rid of the name and IID tables, and she of course already has her followup work rebased on top of this, so we'll just go ahead with her version of things. Once the dust settles from that I'll figure out if the other cleanup bugs I filed are still needed.
Assignee: continuation → nika
No longer depends on: 1450321, 1450359
Attached file semi working patches
For posterity, here are my work in progress patches. There's a bug in part 7, as I note in the commit message. This is on top of hg commit  a1fb8ffae378963b128deaaf3a76eff9dbb6be21, then the patches from bug 1448454, bug 1449747, bug 1450361, bug 1450321 and bug 1450359.
Nika's patches for this apparently also get rid of nsIInterfaceInfo and the dynamic allocation for the hash tables from nsIDs and names to xptiInterfaceEntry. The latter is another 100kb on top of the 100kb from making xptiInterfaceEntry static. The former isn't tracked right now, but with napkin math I'd say that each one is a couple of words of data, and maybe we have on the order of a 100 or so of them.
Blocks: 1451425
Unfortunately, I wasn't able to figure out a way to make firefox build & run in
the intermediate stages of these commits. Because of this, I am going to just
delete most of the code which I am deleting in the first patch, as I figure that
those are somewhat uninteresting changes, and then make the other changes in the
following patches.

In total, the following things are deleted:
 1. All of xpcom/typelib - this directory is being subsumed entirely into xpcom/reflect/xptinfo.
 2. Most of the code in xpcom/reflect/xptinfo, it is being rewritten to avoid allocating and contain all of the necessary data structures.
 3. idl-parser's typelib.py XPT generator, as its xpt dependency is also being removed in this patch, and it will be replaced.
 4. Most includes of files which have been deleted.
Attachment #8965146 - Flags: review?(continuation)
In the previous patch, one of the files which was deleted is ShimInterfaceInfo.
This is an implementor of nsIInterfaceInfo which exists for legacy reasons, in
order to allow Components.interfaces.nsIDOM* to have the correct constants and
IIDs associated with them.

As that file was deleted, this information now has to be stored in the typelib.
To do this, the information is moved to the xptshim and xptshimfile attributes
on the relevant xpcom interfaces.

xptshim(...) means that this xpcom interface is a shim for the WebIDL interface
with the specified name.

xptshimfile(...) is for use when the webidl interface is declared in another
interface's .webidl file, (in our case, MessageManager.webidl). It contains the
name of the parent binding, such that we can #include the correct file in our
generated code.

This patch does not add the code which uses these changes, only the parsing
logic.
Attachment #8965147 - Flags: review?(continuation)
This patch adds a python script based on the old typelib.py script which takes
in a parsed XPIDL file, and generates a json-based XPT file to use as a build
intermediate. I did my best to keep the generated format simple.
Attachment #8965148 - Flags: review?(continuation)
This patch contains the meat of the changes here. The following summarize the changes:
 1. xptinfo.h is rewritten to expose the new interface for reading the XPT data,

    The nsXPTInterfaceInfo object exposes methods with the same signatures as
    the methods on nsIInterfaceInfo, to make converting code which used
    nsIInterfaceInfo as easy as possible, even when those methods don't have
    signatures which make a ton of sense anymore. There are also a few methods
    which are unnecessary (they return `true` or similar), which should be
    removed over time.

    Members of the data structures are made private in order to prevent reading
    them directly. Code should instead call the getter methods. This should make
    it easier to change their memory representation in the future. Constructing
    these structs is made possible by making the structs `friend class` with the
    XPTConstruct class, which is implemented by the code generator, and is able
    to access the private fields.

    In addition, rather than using integers with flag constants, I opted for
    using C++ bitfields to store individual flags, as I found it made it easier
    to both write the code generator, and reason about the layouts of the types.

    I was able to shave a byte off of each nsXPTParamInfo (4 bytes -> 3 bytes)
    by shoving the flags into spare bits in the nsXPTType. Unfortunately there
    was not enough room for the retval flag. Fortunately, we already depend in
    our code on the retval parameter being the last parameter, so I worked
    around this by removing the retval flag and instead having a `hasretval`
    flag on the method itself. 

 2. An xptinfo.cpp file is added for out-of-line definitions of more complex
    methods, and the internal implementation details of the perfect hash.

    Notable is the handling of xptshim interfaces. As the type is uniform, a
    flag is checked when trying to read constant information, and a different
    table with pointers into webidl data structures is checked when the type is
    determined to be a shim.

    Ideally we could remove this once we remove the remaining consumers of the
    existing shim interfaces.

 3. A python code generator which takes in the json XPT files generated in the
    previous part, and emits a xptdata.cpp file with the data structures. I did
    my best to heavily comment the code.

    This code uses the friend class trick to construct the private fields of the
    structs, and avoid a dependency on the ordering of fields in xptinfo.h.

    The sInterfaces array's order is determined by a generated perfect hash
    which is also written into the binary. This should allow for fast lookups by
    IID or name of interfaces in memory. The hash function used for the perfect
    hash is a simple FNV hash, as they're pretty fast.

    For perfect hashing of names, another table is created which contains
    indexes into the sInterfaces table. Lookup by name is less common, and this
    form of lookup should still be very fast.

 4. The necessary Makefiles are updated to use the new code generator, and
    generate the file correctly.
Attachment #8965149 - Flags: review?(continuation)
Due to the decision to keep the old API on nsXPTInterfaceInfo in part 4, this is
a fairly straightforward patch.

 1. I had to change a couple of consumers of `IsRetval()` due to the movement of
    that flag.
 2. I changed all code which held a nsIInterfaceInfo to hold an `const
    nsXPTInterfaceInfo*` instead.
 3. I changed code which used the nsIInterfaceInfoManager to instead call the
    static methods on nsXPTInterfaceInfo.
Attachment #8965150 - Flags: review?(continuation)
I'm really sorry about how gross the patches are :'-(. I couldn't figure out a way to split them up more nicely.

If you'd like the patch pushed to github, LMK.
Are you okay with me reviewing these patches, Nathan?
Flags: needinfo?(nfroyd)
(In reply to Andrew McCreight [:mccr8] from comment #12)
> Are you okay with me reviewing these patches, Nathan?

Yes!
Flags: needinfo?(nfroyd)
FYI my review is in progress. I'm planning to give every patch at least a cursory review before I start marking review on anything, so I can try to understand the way everything works together. I'm also not familiar with anything in idl-parser so changes in there will take more time for me to get through.
Made some updates to these patches to fix build errors.
Attachment #8965864 - Flags: review?(continuation)
Attachment #8965149 - Attachment is obsolete: true
Attachment #8965150 - Attachment is obsolete: true
Attachment #8965149 - Flags: review?(continuation)
Attachment #8965150 - Flags: review?(continuation)
I needed to add another section 'cause I was running into infra failures due to
tests wanting to use the xpt module which is no longer in tree. This patch
cleans up that unused code.

MozReview-Commit-ID: EYbYMREFc0T
Attachment #8965866 - Flags: review?(continuation)
(In reply to Nika Layzell [:mystor] from comment #17)
> Created attachment 8965866 [details] [diff] [review]
> Part 6: Remove XPT files from bundling

I'm not sure this is going to be okay. I had similar changes to this directory in bug 1438688, but Glandium objected, saying that somebody might be using mozpack across versions, so XPT support should be left in. See comment 50 in that bug. I ended up reverting my mozpack changes entirely. I guess your changes break these tests while mine don't, because I didn't change the XPT format or something? You'll have to get review from Glandium on this part, as I'm not sure what is okay or isn't.
(In reply to Andrew McCreight [:mccr8] from comment #18)
> I'm not sure this is going to be okay. I had similar changes to this
> directory in bug 1438688, but Glandium objected, saying that somebody might
> be using mozpack across versions, so XPT support should be left in. See
> comment 50 in that bug. I ended up reverting my mozpack changes entirely. I
> guess your changes break these tests while mine don't, because I didn't
> change the XPT format or something? You'll have to get review from Glandium
> on this part, as I'm not sure what is okay or isn't.

That's unfortunate :-/. If we can't get rid of XPT handling in mozpack, then we'll also have to keep the xpt/tools code around, despite not using XPT files anywhere in our build system.

:glandium, what's the motivation for keeping XPT support in tree when we don't use it?
Flags: needinfo?(mh+mozilla)
I figured it'd be easier & cleaner if I just leave the XPT.py stuff in tree and
don't remove it entirely, so I've restored it. We can clean that stuff up if we
want to later.
Attachment #8966320 - Flags: review?(continuation)
Attachment #8965146 - Attachment is obsolete: true
Attachment #8965866 - Attachment is obsolete: true
Attachment #8965146 - Flags: review?(continuation)
Attachment #8965866 - Flags: review?(continuation)
Comment on attachment 8966320 [details] [diff] [review]
Part 1: Clear out xptinfo and typelib to make way for the this patch

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

nit: "the this" in first line of commit message.

::: dom/xbl/nsXBLPrototypeBinding.cpp
@@ -34,5 @@
>  #include "nsXBLProtoImpl.h"
>  #include "nsCRT.h"
>  #include "nsContentUtils.h"
>  #include "nsTextFragment.h"
> -#include "nsTextNode.h"

The removal of this #include is the cause of the build bustage, though I guess you just fixed it by adding it back in part 5.
Attachment #8966320 - Flags: review?(continuation) → review+
Comment on attachment 8965147 [details] [diff] [review]
Part 2: Add the xptshim and xptshimfile attributes to xpidl

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

It is nice how this means that if you remove an XPIDL interface you are also automatically removing the shim.

kComponentsInterfaceShimMap has an entry for nsIDOMEventTarget and nsITreeBoxObject, but you don’t add a shim for them here. Why is that? If it is intentional, please explain why in the commit message.

Why is it okay to remove "builtinclass" from some of these interfaces? (I haven't read through the Python code in later patches yet, so apologies if it is obvious later.)

I think the names of these attributes should be more like "shim" and "shimfile". XPT is more of an implementation detail than a concept in the XPIDL language. The entire XPIDL MDN page only contains "xpt" twice. Does that sounds reasonable to you?

Could the shimfile argument have the extension? It feels a little more obvious what it means if it is like "shimfile(MessageManager.webidl)". MessageManager isn’t a file, after all. And then check if the extension is webidl somewhere.

It is too bad that the name argument to shim isn’t optional, but I see there’s no current setup for an optional argument to an attribute, and it doesn’t feel worthwhile to add a mechanism for what is fundamentally a temporary mechanism.

Please document xptshim and xptshimfile on https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL Just like one sentence on each or something.

::: dom/base/moz.build
@@ +70,5 @@
>      'nsDOMNavigationTiming.h',
>      'nsDOMString.h',
>      'nsDOMTokenList.h',
>      'nsFocusManager.h',
> +    'nsFrameLoader.h',  # Because binding headers include it.

This looks like it belongs in a later patch that adds something that uses this header. I suppose it doesn't matter where it is added.

::: dom/base/nsIMessageManager.idl
@@ +264,5 @@
>   * sent.  For example, a child-process message manager will send
>   * messages that are only delivered to its one parent-process message
>   * manager.
>   */
> +[xptshim(MessageSender), xptshimfile(MessageManager),

It is unfortunate that you have to introduce this mechanism for a single case, but I guess it is better than hard coding a special case in the XPIDL parser...
Attachment #8965147 - Flags: review?(continuation) → review+
(I've written up most of my review for the C++ code in part 4, but I haven't gone over the Python in 3 and 4 yet, which I need to do before I finish that.)
> Why is it okay to remove "builtinclass" from some of these interfaces? (I haven't read through the Python code in later patches yet, so apologies if it is obvious later.)

To answer my own question, one of the changes these patches make is that shim classes are now implicitly builtinclass.
(In reply to Andrew McCreight [:mccr8] from comment #21)
> The removal of this #include is the cause of the build bustage, though I
> guess you just fixed it by adding it back in part 5.

Oops D:


(In reply to Andrew McCreight [:mccr8] from comment #22)
> It is nice how this means that if you remove an XPIDL interface you are also
> automatically removing the shim.
> 
> kComponentsInterfaceShimMap has an entry for nsIDOMEventTarget and
> nsITreeBoxObject, but you don’t add a shim for them here. Why is that? If it
> is intentional, please explain why in the commit message.

I don't remember why I didn't include those here. I looked in our tree & we don't use the shims at all in our code right now, so we can probably get away with leaving them out.

> Why is it okay to remove "builtinclass" from some of these interfaces? (I
> haven't read through the Python code in later patches yet, so apologies if
> it is obvious later.)

Shims are, by definition, not scriptable, so they are also inherently builtinclass.

> I think the names of these attributes should be more like "shim" and
> "shimfile". XPT is more of an implementation detail than a concept in the
> XPIDL language. The entire XPIDL MDN page only contains "xpt" twice. Does
> that sounds reasonable to you?

Sure - I can call it shim and shimfile or something like that.

> Could the shimfile argument have the extension? It feels a little more
> obvious what it means if it is like "shimfile(MessageManager.webidl)".
> MessageManager isn’t a file, after all. And then check if the extension is
> webidl somewhere.

Yeah, I could do something like that. I didn't bother 'cause I was kinda hoping to just get rid of the uses of shimfile & rip it out ASAP D:. Wasn't really thinking much about asthetics.

> It is too bad that the name argument to shim isn’t optional, but I see
> there’s no current setup for an optional argument to an attribute, and it
> doesn’t feel worthwhile to add a mechanism for what is fundamentally a
> temporary mechanism.
> 
> Please document xptshim and xptshimfile on
> https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL Just like one
> sentence on each or something.

Sure

> ::: dom/base/moz.build
> @@ +70,5 @@
> >      'nsDOMNavigationTiming.h',
> >      'nsDOMString.h',
> >      'nsDOMTokenList.h',
> >      'nsFocusManager.h',
> > +    'nsFrameLoader.h',  # Because binding headers include it.
> 
> This looks like it belongs in a later patch that adds something that uses
> this header. I suppose it doesn't matter where it is added.

This is used technically when we start generating code. One of the bindings which we have to connect to is FrameLoaderBinding.h, which includes nsFrameLoader.h.

I don't remember how the change ended up here, but this patchset doesn't build in any intermediate state anyway, so I'm not sure it's worth fixing.

> ::: dom/base/nsIMessageManager.idl
> @@ +264,5 @@
> >   * sent.  For example, a child-process message manager will send
> >   * messages that are only delivered to its one parent-process message
> >   * manager.
> >   */
> > +[xptshim(MessageSender), xptshimfile(MessageManager),
> 
> It is unfortunate that you have to introduce this mechanism for a single
> case, but I guess it is better than hard coding a special case in the XPIDL
> parser...

Yeah, that was my reasoning. I'd ideally want to move toward getting rid of these shims so we can rip out shimfile.
Comment on attachment 8965148 [details] [diff] [review]
Part 3: Replace the XPT file format with a JSON based one

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

This cleans out a nice amount of gunk. It was a little hard figuring out how this all worked, because I had to look at the original file, this patch, and the next patch to trace where some things went.

::: python/mozbuild/mozbuild/action/xpidl-process.py
@@ +66,2 @@
>      xpt_path = os.path.join(xpt_dir, '%s.xpt' % module)
> +    with FileAvoidWrite(xpt_path) as fh:

So, this breaks incremental builds, in a probably inconsequential manner. If you rename an XPCOM module in a moz.build file from foo to bar, then do a build, it'll generate the new bar.xpt file. Then if you change it back, the build system will notice that foo.xpt didn't change, and not build xptdata.cpp again. But I guess that's okay? The module name doesn't affect xptdata.cpp anything as far as I remember. If you changed anything in the module while it was bar.xpt, then foo.xpt will change and it will be fine. Anyways, I just thought I'd mention that in case you can think of some way that will cause a problem.

::: xpcom/idl-parser/xpidl/jsonxpt.py
@@ +1,2 @@
> +#!/usr/bin/env python
> +# jsonxpt.py - Generate json XPT typelib files from IDL.

nit: JSON

@@ +4,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +"""Generate a json XPT typelib for an IDL file"""

nit: JSON

@@ +12,5 @@
> +import xpidl
> +import json
> +import itertools
> +
> +# A map of xpidl.py types to xpt enum variants

Please mention the name of the C++ type you are mapping to.

@@ +60,5 @@
> +        ret = { 'tag': TypeMap[type.name] }
> +        if type.name in ['string', 'wstring'] and size_is is not None:
> +            ret['tag'] += '_SIZE_IS'
> +            ret['size_is'] = size_is
> +        return ret

Ah, nice, I didn't realize all of that isPtr and isRef stuff wasn't even used in C++. When I was looking at this code, I just preserved all of the bits and didn't notice that kFlagMask was wiping out those parts.

@@ +184,5 @@
> +            param = mk_param(get_type(a.realtype, 'in'), in_=1)
> +            methods.append(mk_method(a.name, [param], setter=1, hidden=a.noscript,
> +                                     context=a.implicit_jscontext))
> +
> +    implicit_builtinclass = False

As an aside, this notion of implicit builtinclass for interfaces that are stubs or have notxpcom methods is inconsistent with how infallible requires explicit builtinclass, but of course this predates your patch.

@@ +211,5 @@
> +        'methods': methods,
> +        'consts': consts,
> +        'parent': iface.base,
> +        'xptshim': iface.attributes.xptshim,
> +        'xptshimfile': iface.attributes.xptshimfile,

There should be an error (in the code in part 2?) if xptshimfile is set, but xptshim is not.
Attachment #8965148 - Flags: review?(continuation) → review+
Two things you should look at, if you haven't already, are installer size and, on Windows, compile time. I had to back out bug 1448454 because it caused a large regression in both on Windows. All I did in that patch was make fields private and use a lot of constexpr Ctors, so your patch may also have similar problems.

Linux doesn't seem to have that same problem with constexprs. I checked out the installer size locally in a Clang opt build.

without patches:
~/mc/obj-opt.noindex/dist$ ls -l firefox-61.0a1.en-US.linux-x86_64.tar.bz2
-rw-rw-r-- 1 amccreight amccreight 68717908 Apr 10 16:36 firefox-61.0a1.en-US.linux-x86_64.tar.bz2

with patches:
~/mc/obj-opt.noindex/dist$ ls -l firefox-61.0a1.en-US.linux-x86_64.tar.bz2
-rw-rw-r-- 1 amccreight amccreight 68753328 Apr 10 16:28 firefox-61.0a1.en-US.linux-x86_64.tar.bz2

That's a 35,420 byte regression. I'm not sure exactly what that means for the actual libxul size, or what is okay or not. Some regression is probably expected due to having these hash tables around that we didn't before. I'll see if I can think of any size reductions we can do when I look at xptcodegen.py.
(In reply to Andrew McCreight [:mccr8] from comment #27)
> Two things you should look at, if you haven't already, are installer size
> and, on Windows, compile time. I had to back out bug 1448454 because it
> caused a large regression in both on Windows. All I did in that patch was
> make fields private and use a lot of constexpr Ctors, so your patch may also
> have similar problems.

Hmm. If constexpr ctors are going to be a problem, I can just generate struct literals & move the statics into a friend class.

I'll double-check and if I'm seeing the regression, I'll do that.

> Linux doesn't seem to have that same problem with constexprs. I checked out
> the installer size locally in a Clang opt build.
> 
> without patches:
> ~/mc/obj-opt.noindex/dist$ ls -l firefox-61.0a1.en-US.linux-x86_64.tar.bz2
> -rw-rw-r-- 1 amccreight amccreight 68717908 Apr 10 16:36
> firefox-61.0a1.en-US.linux-x86_64.tar.bz2
> 
> with patches:
> ~/mc/obj-opt.noindex/dist$ ls -l firefox-61.0a1.en-US.linux-x86_64.tar.bz2
> -rw-rw-r-- 1 amccreight amccreight 68753328 Apr 10 16:28
> firefox-61.0a1.en-US.linux-x86_64.tar.bz2
> 
> That's a 35,420 byte regression. I'm not sure exactly what that means for
> the actual libxul size, or what is okay or not. Some regression is probably
> expected due to having these hash tables around that we didn't before. I'll
> see if I can think of any size reductions we can do when I look at
> xptcodegen.py.

Huh, that's a pretty big regression. I'll definitely have to look into what's causing it.

Based on the sizes of the hashtables, the regression shouldn't be nearly that large. The tables should only be 2x256 uint32_t and ~1200ish uint16_t so only ~4.5k bytes. Perhaps this increase was caused by more code being inlined into callers? Not sure.
(In reply to Nika Layzell [:mystor] from comment #28)
> Huh, that's a pretty big regression. I'll definitely have to look into
> what's causing it.
Great!

> Based on the sizes of the hashtables, the regression shouldn't be nearly
> that large. The tables should only be 2x256 uint32_t and ~1200ish uint16_t
> so only ~4.5k bytes. Perhaps this increase was caused by more code being
> inlined into callers? Not sure.

Yeah, after I posted that I looked at the file and saw how tiny the hash table data was. Maybe you didn't implement some of my weird little optimizations for reducing data? As a first step, just check out the length of each data structure with and without the patches.
Comment on attachment 8965864 [details] [diff] [review]
Part 4: Rewrite xptinfo, and write a new xptcodegen.py to generate the required datastructures

Eric, could you please review the perfect hash generator stuff? That's phf.py, the "Perfect Hash Function backing data" section in xpt_struct.h, "C++ Perfect Hash Helper Functions" and maybe "PHF-based interface lookup methods" in xptinfo.cpp. Thanks.
Attachment #8965864 - Flags: review?(erahm)
(In reply to Andrew McCreight [:mccr8] from comment #29)
> Maybe you didn't implement some of my weird little
> optimizations for reducing data? As a first step, just check out the length
> of each data structure with and without the patches.

I looked at how many of each kind of thing are being stored with and without your patch. The number of interfaces, consts, and methods match, as you'd expect. However, params (which are a pretty big chunk of the total data) go from around 5900 to 11000. The mega string goes from around 153000 characters to 162900 characters. So that probably explains a decent amount of the increase.
This is a smaller amount, but I also see that the number of top-level types goes from about 40 to about 200 with your patch. I think there the issue is just that you are missing deduplication for types. For instance, uint8 appears about 40 times.

This is a pre-existing issue, so I'd leave it for followup work unless you really need it, but I wonder if using a type index instead of an inline type in parameters would save space. The index is smaller, but maybe it wouldn't be if we needed a lot more top level types.
Comment on attachment 8965864 [details] [diff] [review]
Part 4: Rewrite xptinfo, and write a new xptcodegen.py to generate the required datastructures

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

Over all, this looks great. I'm r-ing it because I want to see how you address the various changes I'm requesting. I also want to make sure the size regressions are addressed, though I suspect that if you add back in the various optimizations I mention below it will be fine.

"There are also a few methods which are unnecessary (they return `true` or similar), which should be removed over time."
Maybe file a bug with some details about this, while it is fresh in your mind?

I used njn's script to look at what is in rodata, and everything is as I'd expect. I also ran the profiler on xptcodegen.py. It runs a little slower than xpt.py, about 2.2 seconds instead of about a second, but the extra time is basically taken up by the perfect hash stuff, so that seems totally reasonable. Spending an extra second for an incremental build that touches an IDL file sounds reasonable to me.

::: config/makefiles/xpidl/Makefile.in
@@ -29,5 @@
>  dist_idl_dir := $(DIST)/idl
>  dist_include_dir := $(DIST)/include
>  dist_xpcrs_dir := $(DIST)/xpcrs
>  process_py := $(topsrcdir)/python/mozbuild/mozbuild/action/xpidl-process.py
> -generated_file := $(topobjdir)/xpcom/typelib/xpt/XPTInfo.cpp

These Makefile changes look trivial (file renames, and changing how a Python command is invoked), so I think it is okay that I review this.

Please file a bug in the build system component about updating tup for this and CC mshal and chmanchester. (Akin to bug 1450877.)

::: xpcom/reflect/xptinfo/phf.py
@@ +1,2 @@
> +#!/usr/bin/env python
> +# phf.py - Helper for generating perfect hash functions for xptcodegen.py

I'll defer to erahm on this, but please give this file a longer name. "phf" is cryptic. Maybe "hashgen.py".

::: xpcom/reflect/xptinfo/xptcodegen.py
@@ +64,5 @@
> +        return r
> +
> +
> +class Constructor(object):
> +    # Helper object for defining and using constexpr methods create xpt types.

nit: "to create".

@@ +125,5 @@
> +        return 0
> +
> +    # NOTE: State used while linking. This is done with closures rather than a
> +    # class due to how this file's code evolved.
> +    includes = []

Maybe rename this to shim_includes so it is more obvious that they are only needed for shims?

@@ +156,5 @@
> +        else:
> +            strings[s] = 0
> +        return strings[s]
> +
> +    def describe_type(type): # Create the type's documentation comment.

I'm glad to see that you were less lazy than me about having decent in-file documentation.

@@ +165,5 @@
> +        elif tag == 'interface_type':
> +            return type['name']
> +        elif tag == 'interface_is_type':
> +            return 'iid_is(%d)' % type['iid_is']
> +        elif tag.endswith('_size_is'):

I like how 400 lines of type subclasses are boiled down into a couple of lines. (Of course, this code is really only doing the code_gen part of it.)

@@ +181,5 @@
> +            try:
> +                d2 = types.index(elty)
> +            except:
> +                d2 = len(types)
> +                types.append(elty)

I think you should add deduplication for top-level types. There's a decent amount of it.

@@ +229,5 @@
> +        methods.append(struct(
> +            "nsXPTMethodInfo",
> +            "%d = %s" % (len(methods), methodname),
> +            {
> +                'mName': lower_string(method['name']),

I was able to also not store the name of the method in the case of hideparams. (See Method::code_gen in xpt.py.) Is that not possible here? This is likely responsible for some of the extra string data with your patches. For instance, on m-c, the XPT data does not contain the string showURIForInput (from the hidden method nsIGIOService), whereas with your patch that is present.

@@ +249,5 @@
> +        ))
> +
> +        if not hideparams:
> +            for idx, param in enumerate(method['params']):
> +                lower_param(param, "%s[%d]" % (methodname, idx))

As I said in a bug comment, you should deduplicate lists of parameters. There's a lot of them! The drawback of course is that you can't easily comment which method the parameters are from. I think that's a reasonable tradeoff to save lots of space in the binary. I guess you could list every method name that got merged into a single parameter list if you want, but I don't know how useful that is vs how much work it'll take.

@@ +285,5 @@
> +        methods = 0
> +        consts = 0
> +        builtinclass = False
> +        while iface is not None:
> +            methods += len(iface['methods'])

I was worried about the performance of computing parent class values over and over again, but it didn't seem to show up in a profiler run I did. I guess our XPIDL class hierarchies are not very deep!

@@ +296,5 @@
> +            iface = iid_phf.values[idx - 1]
> +
> +        return methods, consts, builtinclass
> +
> +    def lower_iface(iface):

Somewhere in here you need to add the equivalent of the check in XPTInterfaceInfoManager::VerifyAndAddEntryIfNew() that if the interface is not builtinclass, the number of methods is <= 250. Also, include the equivalent of that comment here.

You should also update the comment in xpcom/reflect/xptcall/genstubs.pl that talks about xptiInterfaceInfoManager.cpp to refer to this file instead.

@@ +311,5 @@
> +            # as they will be pulled from the WebIDL binding instead. Instead,
> +            # we use the constants offset field to store the index into the prop
> +            # hooks table.
> +            consts_off = len(prophooks)
> +            builtinclass = True  # All shims are builtinclass

I think it would be better if builtinclass was set to True for shims in build_interface in jsonxpt.py. You could assert for it here if you want. But there's already implicit_builtinclass there, and it would be nicer if that was all in one place.

@@ +357,5 @@
> +    # Include any bindings files which we need to include due to XPT shims.
> +    for include in includes:
> +        fd.write('#include "%s"\n' % include)
> +
> +    # Write our out header

nit: "out our".

@@ +382,5 @@
> +    #  1. Writing constructors this way means that xptcodegen.py is not
> +    #     dependent on the order of fields in xptinfo.h
> +    #  2. As a member of the XPTConstruct struct, we are `friend class` of the
> +    #     data structures, allowing us to initialize private fields.
> +    #  3. Stating the name of fields in this python code makes it more

nit: Python.

@@ +384,5 @@
> +    #  2. As a member of the XPTConstruct struct, we are `friend class` of the
> +    #     data structures, allowing us to initialize private fields.
> +    #  3. Stating the name of fields in this python code makes it more
> +    #     self-documenting.
> +    fd.write("struct XPTConstruct {")

Should this class have an nsFoo-style name? It seems like the remaining class names are all like that.

@@ +400,5 @@
> +    array("nsXPTMethodInfo", "sMethods", methods)
> +    array("ConstInfo", "sConsts", consts)
> +    array("mozilla::dom::NativePropertyHooks*", "sPropHooks", prophooks)
> +
> +    # The strings array. We write out individual characters to avoid msvc restrictions.

nit: MSVC

@@ +434,5 @@
> +} // namespace xpt
> +""")
> +
> +
> +def link_and_write(files, outfile):

xpt.py has some optimization that dumps all non-scriptable interfaces that aren't referred to by scriptable interfaces. It looks like instead you unilaterally drop all non-scriptable interfaces in jsonxpt.py. I don't want to dig through bug 996061 to figure out why they had to keep the non-scriptable interfaces around, so I guess it is okay if try is green. :) I guess my optimization to drop method info from notxpcom methods is similar in spirit to that.

::: xpcom/reflect/xptinfo/xptinfo.cpp
@@ +81,5 @@
> +GetWebIDLConst(uint16_t aHookIdx, uint16_t aIndex, const ConstantSpec** aSpec)
> +{
> +  const NativePropertyHooks* propHooks = sPropHooks[aHookIdx];
> +
> +  uint16_t idx;

idx looks like it is uninitialized here.

@@ +96,5 @@
> +          // calling isEnabled() here because it's OK to define potentially
> +          // extra constants on these shim interfaces.
> +          if (aSpec && idx == aIndex) {
> +            *aSpec = cs;
> +            return idx;

It is nice that you avoided the copy and paste stuff here, but it makes me uneasy how you are returning a value here that is never used, so it is untested code. Maybe make the two separate modes more explicit by returning the number of constants via a possibly-null argument, like it is done for aSpec? Then you can assert that only one or the other is being used, and thus there will be no untested calculations.

@@ +134,5 @@
> +    }
> +
> +    // Extract the value and name from the Constant Info.
> +    const ConstInfo& info = sConsts[mConsts + aIndex];
> +    if (info.mSigned || info.mValue <= (uint32_t)INT32_MAX) {

I'm not sure I 100% understand this but looking at the old code it seems reasonable.

@@ +285,5 @@
> +  return NS_ERROR_FAILURE;
> +}
> +
> +nsresult
> +nsXPTInterfaceInfo::GetTypeForParam(uint16_t aMethodIndex,

It feels strange that aMethodIndex is unused here and in the next few methods. I suppose that this is fallout from my earlier hoisting of parameter data to a global structure instead of a per-method structure. Maybe add a comment saying that it is unused? Technically your patch changes behavior in the case where somebody passes in an invalid index, but I don't think that's a real concern.

::: xpcom/reflect/xptinfo/xptinfo.h
@@ +20,5 @@
>  
> +// Forward Declarations
> +namespace mozilla {
> +namespace dom {
> +class NativePropertyHooks;

nit: this should be |struct|, not |class|. I get a lot of build warnings from this.

@@ +34,5 @@
> +namespace detail {
> +
> +// xpt::detail::XPTConstruct is friend class with each of the data classes, such
> +// that the generated code can access private fields.
> +class XPTConstruct;

XPTConstruct is a struct here, but a class in xptdata.cpp. I'm getting a warning about that. This probably also applies to the friend class declarations below.

@@ +37,5 @@
> +// that the generated code can access private fields.
> +class XPTConstruct;
> +
> +inline const nsXPTInterfaceInfo* GetInterface(uint16_t aIndex);
> +inline const nsXPTType& GetType(uint16_t aIdx);

Please be consistent about aIdx vs aIndex. (I prefer the latter myself, and it appears to be more common in the code base, but either is fine.)

@@ +42,5 @@
> +inline const nsXPTParamInfo& GetParam(uint16_t aIdx);
> +inline const nsXPTMethodInfo& GetMethod(uint16_t aIdx);
> +inline const char* GetString(uint32_t aIdx);
> +
> +extern const uint16_t sInterfacesSize;

You declare this a second time down below, which seems a little weird. I guess you have to forward declare it because it appears in code in the header, but it would also be odd to not mention it down with the rest of the static variables...

@@ +49,5 @@
> +} // namespace xpt
> +
> +
> +/*
> + * An Interface describes a single XPCOM interface, including all of its

Maybe "nsXPTInterfaceInfo" instead of "Interface" here?

@@ +82,5 @@
> +    return xpt::detail::GetInterface(mParent);
> +  }
> +
> +  // Do we have an ancestor interface with the given IID?
> +  bool HasAncestor(const nsIID& aIID) const {

OOC, why are you defining this and Method() inline?

@@ +122,5 @@
> +  // nsIInterfaceInfo backwards compatibility //
> +  //////////////////////////////////////////////
> +
> +  nsresult GetName(char** aName) const;
> +  nsresult GetInterfaceIID(nsIID** aIID) const;

I deleted this method, along with GetInfoForParam and GetIIDForParam, in bug 1449747, so you can remove them here.

@@ +129,5 @@
> +  nsresult GetParent(const nsXPTInterfaceInfo** aParent) const;
> +  nsresult GetMethodCount(uint16_t* aMethodCount) const;
> +  nsresult GetConstantCount(uint16_t* aConstantCount) const;
> +  nsresult GetMethodInfo(uint16_t aIndex, const nsXPTMethodInfo** aInfo) const;
> +  nsresult GetMethodInfoForName(const char* aMethodName, uint16_t* aIndex,

GetMethodInfoForName and MethodByName are unused, so you should delete them rather than convert them. (I'm not sure when that happened. I'm pretty sure they were used a week or two ago!)

@@ +158,5 @@
> +                                 const nsXPTParamInfo* aParam,
> +                                 nsIID* aIID) const;
> +  nsresult IsMainProcessScriptableOnly(bool* aRetval) const;
> +
> +  // XXX(nika): Remove?

Maybe expand on this comment?

@@ +159,5 @@
> +                                 nsIID* aIID) const;
> +  nsresult IsMainProcessScriptableOnly(bool* aRetval) const;
> +
> +  // XXX(nika): Remove?
> +  bool EnsureResolved() const { return !mIsShim; }

Ah, good point. I didn't think about how shim interfaces act like they are unresolved.

@@ +164,5 @@
> +
> +private:
> +  friend class xpt::detail::XPTConstruct;
> +  constexpr nsXPTInterfaceInfo()
> +    : mIID{0}, mName{0}, mParent{0}, mBuiltinClass{0}

Out of curiosity, why the {} instead of ()?

@@ +172,5 @@
> +  /*
> +   * This field ordering minimizes the size of this struct.
> +   */
> +  nsID mIID;
> +  uint32_t mName; // Index into XPTHeader::mStrings.

Most or all of these "Index into" comments need to be updated. For instance, I think this is actually an index into xpt::detail::sStrings.

@@ +176,5 @@
> +  uint32_t mName; // Index into XPTHeader::mStrings.
> +
> +  uint16_t mParent : 14;
> +  uint16_t mBuiltinClass : 1;
> +  // XXX(nika): Do we need this if we don't have addons anymore?

Please file a bug about maybe removing this.

@@ +182,5 @@
> +
> +  uint16_t mMethods; // Index into XPT::sMethods.
> +
> +  uint16_t mConsts : 14; // Index into XPT::sConsts.
> +  uint16_t mIsShim : 1; // Is this interface a ShimInterface?

ShimInterface doesn't exist any more. :) Maybe something like "Is this interface a WebIDL shim interface?" Is there any in tree documentation of what a shim interface is, now that ShimInterface.* is gone?

@@ +191,3 @@
>  };
>  
> +static_assert(sizeof(nsXPTInterfaceInfo) == 28, "wrong size?");

Please document what depends on this being that size, if anything. Likewise with the other static asserts.

@@ +231,5 @@
> +};
> +
> +
> +/*
> + * A TypeDescriptor is a union used to identify the type of a method

nit: "TypeDescriptor" should be "nsXPTType" in this comment, I think?

@@ +256,5 @@
> +    return xpt::detail::GetType(mData2);
> +  }
> +
> +  // We store the 16-bit iface value as two 8-bit values in order to
> +  // avoid 16-bit alignment requirements for XPTTypeDescriptor, which

nit: The two type names in this comment need to be updated. Likewise with later comments.

@@ +293,5 @@
> +    return Tag() == TD_INTERFACE_IS_TYPE || Tag() == TD_ARRAY ||
> +           Tag() == TD_PSTRING_SIZE_IS || Tag() == TD_PWSTRING_SIZE_IS;
> +  }
> +
> +  bool IsSizedString() const {

I think this method is unused.

@@ +310,5 @@
> +  constexpr nsXPTType()
> +    : mTag{0}, mInParam{0}, mOutParam{0}, mOptionalParam{0}, mData1{0}, mData2{0}
> +  { }
> +  MOZ_IMPLICIT nsXPTType(const uint8_t& aPrefix)
> +    : mTag(aPrefix) { MOZ_ASSERT(aPrefix <= TD_JSVAL); }

Should the other fields be initialized, too? Also, this uses () instead of {}, if that matters.

@@ +370,5 @@
> +static_assert(sizeof(nsXPTType) == 3, "wrong size");
> +
> +
> +/*
> + * A ParamDescriptor is used to describe either a single argument to a method or

nsXPTParamInfo, not ParamDescriptor.

@@ +379,5 @@
>  public:
> +  bool IsIn() const { return mType.mInParam; }
> +  bool IsOut() const { return mType.mOutParam && !IsDipper(); }
> +  bool IsOptional() const { return mType.mOptionalParam; }
> +  bool IsShared() const { return false; } // XXX remove (backcompat)

Should IsScriptable() above have the full "// XXX remove (backcompat)" comment? Or would it be harder to remove?

@@ +398,5 @@
> +  // parameters masquerading as in'. The burden of maintaining this illusion
> +  // falls mostly on XPConnect, which creates the empty containers, and harvest
> +  // the results after the call.
> +  //
> +  // Currently, the only dipper types are the string classes.

Ah, I see you moved the calculation of what is a dipper type from typelib.py to here. You should mention bug 677784 here, then.

@@ +418,5 @@
> +
> +static_assert(sizeof(nsXPTParamInfo) == 3, "wrong size");
> +
> +/*
> + * A MethodInfo is used to describe a single interface method.

nit: nsXPTMethodInfo

@@ +500,5 @@
>  };
> +static_assert(sizeof(ConstInfo) == 8, "wrong size");
> +
> +//////////////////////////////////////////////
> +// Raw typelib data stored in const statics //

These aren't marked as static here, per se. Also, I found it vaguely confusing for "const" to suddenly mean the C++ keyword right after the class ConstInfo but maybe that is just me. :) Maybe throw in an explanation here like "to avoid relocations"?

@@ +514,5 @@
> +extern const char sStrings[];
> +extern const ConstInfo sConsts[];
> +
> +// shim constant information
> +extern const mozilla::dom::NativePropertyHooks* sPropHooks[];

Just to double check here, this'll basically be about the same amount of relocatable data as we had with kComponentsInterfaceShimMap? (A little less, even!)
Attachment #8965864 - Flags: review?(continuation) → review-
Comment on attachment 8965865 [details] [diff] [review]
Part 5: Update consumers of nsIInterfaceInfo to use the nsXPTInterfaceInfo directly

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

Cool! I'm glad this horrible interface is gone.

::: dom/xbl/nsXBLPrototypeBinding.cpp
@@ +730,5 @@
>            mInterfaceTable.Put(*iid, mBinding);
>  
>            // this block adds the parent interfaces of each interface
>            // defined in the xbl definition (implements="nsI...")
> +          const nsXPTInterfaceInfo* parentInfo;

Maybe initialize this to null just to make it safer?

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1399,5 @@
>  CallMethodHelper::GetOutParamSource(uint8_t paramIndex, MutableHandleValue srcp) const
>  {
>      const nsXPTParamInfo& paramInfo = mMethodInfo->GetParam(paramIndex);
>  
> +    bool isRetval = mMethodInfo->HasRetval() &&

Please wrap this up into a predicate method on method info and use it everywhere. This seems a little finicky and hard to understand to have scattered around.

::: xpcom/reflect/xptcall/genstubs.pl
@@ +15,5 @@
>  # 1) The current Linux ARM code has a limitation of only having 256-3 stubs,
>  #    as a result of the limitations of immediate values in ARM assembly.
>  #
>  # This number is verified by the IDL parser in xpcom/idl-parser/xpidl.py, as
> +# well as in xpcom/reflect/xptinfo/xptinfo.cpp, to prevent generating interfaces

Ah, I see you updated this file in this patch rather than the previous one.
Attachment #8965865 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #33)
> Over all, this looks great. I'm r-ing it because I want to see how you
> address the various changes I'm requesting. I also want to make sure the
> size regressions are addressed, though I suspect that if you add back in the
> various optimizations I mention below it will be fine.

Yeah :-). I've been working on optimizing the parameters etc. better :-). So far the changes look promising.

> "There are also a few methods which are unnecessary (they return `true` or
> similar), which should be removed over time."
> Maybe file a bug with some details about this, while it is fresh in your
> mind?

Sure, I'll file something when I get closer to landing this.

> I used njn's script to look at what is in rodata, and everything is as I'd
> expect. I also ran the profiler on xptcodegen.py. It runs a little slower
> than xpt.py, about 2.2 seconds instead of about a second, but the extra time
> is basically taken up by the perfect hash stuff, so that seems totally
> reasonable. Spending an extra second for an incremental build that touches
> an IDL file sounds reasonable to me.

Yeah, I've been running the python profiler on the code on-and-off and the performance difference has pretty much been in the PHF implementation.

If we wanted to be as efficient as possible with PHF generation, we could use gperf rather than my code, but I don't think we want to add a new build dependency for Firefox right now, which is why I avoided doing so.

> ::: config/makefiles/xpidl/Makefile.in
> @@ -29,5 @@
> >  dist_idl_dir := $(DIST)/idl
> >  dist_include_dir := $(DIST)/include
> >  dist_xpcrs_dir := $(DIST)/xpcrs
> >  process_py := $(topsrcdir)/python/mozbuild/mozbuild/action/xpidl-process.py
> > -generated_file := $(topobjdir)/xpcom/typelib/xpt/XPTInfo.cpp
> 
> These Makefile changes look trivial (file renames, and changing how a Python
> command is invoked), so I think it is okay that I review this.
> 
> Please file a bug in the build system component about updating tup for this
> and CC mshal and chmanchester. (Akin to bug 1450877.)

OK

> ::: xpcom/reflect/xptinfo/phf.py
> @@ +1,2 @@
> > +#!/usr/bin/env python
> > +# phf.py - Helper for generating perfect hash functions for xptcodegen.py
> 
> I'll defer to erahm on this, but please give this file a longer name. "phf"
> is cryptic. Maybe "hashgen.py".

Fair. I'll probably call it perfecthash.py so make it clear what's going on. In other libraries I usually see it referred to by its short PHF acronym, which is why I used that here :-).

> @@ +125,5 @@
> > +        return 0
> > +
> > +    # NOTE: State used while linking. This is done with closures rather than a
> > +    # class due to how this file's code evolved.
> > +    includes = []
> 
> Maybe rename this to shim_includes so it is more obvious that they are only
> needed for shims?

They're also used in the WebIDL in XPIDL bug for those includes, so I didn't name it as shim_*.

> @@ +156,5 @@
> > +        else:
> > +            strings[s] = 0
> > +        return strings[s]
> > +
> > +    def describe_type(type): # Create the type's documentation comment.
> 
> I'm glad to see that you were less lazy than me about having decent in-file
> documentation.

I put entirely too much effort into in-file documentation TBH :p

> @@ +165,5 @@
> > +        elif tag == 'interface_type':
> > +            return type['name']
> > +        elif tag == 'interface_is_type':
> > +            return 'iid_is(%d)' % type['iid_is']
> > +        elif tag.endswith('_size_is'):
> 
> I like how 400 lines of type subclasses are boiled down into a couple of
> lines. (Of course, this code is really only doing the code_gen part of it.)

^_^ - The joys of shedding off baggage - sometimes stuff becomes way simpler because you're no longer shoehorning a square peg into a round hole :-)

> @@ +181,5 @@
> > +            try:
> > +                d2 = types.index(elty)
> > +            except:
> > +                d2 = len(types)
> > +                types.append(elty)
> 
> I think you should add deduplication for top-level types. There's a decent
> amount of it.

They actually should be deduplicated, but when I added the `Instance` class I forgot to add an __eq__ implementation for it, so the duplication test never succeeded >.<

Oops - will fix that.

> @@ +229,5 @@
> > +        methods.append(struct(
> > +            "nsXPTMethodInfo",
> > +            "%d = %s" % (len(methods), methodname),
> > +            {
> > +                'mName': lower_string(method['name']),
> 
> I was able to also not store the name of the method in the case of
> hideparams. (See Method::code_gen in xpt.py.) Is that not possible here?
> This is likely responsible for some of the extra string data with your
> patches. For instance, on m-c, the XPT data does not contain the string
> showURIForInput (from the hidden method nsIGIOService), whereas with your
> patch that is present.

Yeah I can do that - didn't occur to me to hide those too :-).

One of the side effects of me writing these patches partially in parallel to your original patches is that I missed some of the clever optimizations you ended up implementing.

> @@ +249,5 @@
> > +        ))
> > +
> > +        if not hideparams:
> > +            for idx, param in enumerate(method['params']):
> > +                lower_param(param, "%s[%d]" % (methodname, idx))
> 
> As I said in a bug comment, you should deduplicate lists of parameters.
> There's a lot of them! The drawback of course is that you can't easily
> comment which method the parameters are from. I think that's a reasonable
> tradeoff to save lots of space in the binary. I guess you could list every
> method name that got merged into a single parameter list if you want, but I
> don't know how useful that is vs how much work it'll take.

I have a local patch to do this, but it still needs some cleanup before I post it.

> @@ +285,5 @@
> > +        methods = 0
> > +        consts = 0
> > +        builtinclass = False
> > +        while iface is not None:
> > +            methods += len(iface['methods'])
> 
> I was worried about the performance of computing parent class values over
> and over again, but it didn't seem to show up in a profiler run I did. I
> guess our XPIDL class hierarchies are not very deep!

Yup! I can also cache them fairly easily, I just didn't bother.

> @@ +296,5 @@
> > +            iface = iid_phf.values[idx - 1]
> > +
> > +        return methods, consts, builtinclass
> > +
> > +    def lower_iface(iface):
> 
> Somewhere in here you need to add the equivalent of the check in
> XPTInterfaceInfoManager::VerifyAndAddEntryIfNew() that if the interface is
> not builtinclass, the number of methods is <= 250. Also, include the
> equivalent of that comment here.
> 
> You should also update the comment in xpcom/reflect/xptcall/genstubs.pl that
> talks about xptiInterfaceInfoManager.cpp to refer to this file instead.

As you noted, I do the updated comment in the other code.

At one point I had the 250 method check here, but it seems like it got deleted in a refactoring - I'll re-add it.

> @@ +311,5 @@
> > +            # as they will be pulled from the WebIDL binding instead. Instead,
> > +            # we use the constants offset field to store the index into the prop
> > +            # hooks table.
> > +            consts_off = len(prophooks)
> > +            builtinclass = True  # All shims are builtinclass
> 
> I think it would be better if builtinclass was set to True for shims in
> build_interface in jsonxpt.py. You could assert for it here if you want. But
> there's already implicit_builtinclass there, and it would be nicer if that
> was all in one place.

Yeah, I can do that.

> @@ +384,5 @@
> > +    #  2. As a member of the XPTConstruct struct, we are `friend class` of the
> > +    #     data structures, allowing us to initialize private fields.
> > +    #  3. Stating the name of fields in this python code makes it more
> > +    #     self-documenting.
> > +    fd.write("struct XPTConstruct {")
> 
> Should this class have an nsFoo-style name? It seems like the remaining
> class names are all like that.

The reasoning was that this data structure is in xpt::detail::* as it was internal, so I didn't give it the same naming style. I agree that it would probably be cleaner to toss that namespace & just call everything nsXPT*, so I'll probably do that.

> @@ +434,5 @@
> > +} // namespace xpt
> > +""")
> > +
> > +
> > +def link_and_write(files, outfile):
> 
> xpt.py has some optimization that dumps all non-scriptable interfaces that
> aren't referred to by scriptable interfaces. It looks like instead you
> unilaterally drop all non-scriptable interfaces in jsonxpt.py. I don't want
> to dig through bug 996061 to figure out why they had to keep the
> non-scriptable interfaces around, so I guess it is okay if try is green. :)
> I guess my optimization to drop method info from notxpcom methods is similar
> in spirit to that.

IIRC there were no test problems with this particular decision. There's nothing that XPConnect code can do with unresolved interfaces anyway, so...

> @@ +96,5 @@
> > +          // calling isEnabled() here because it's OK to define potentially
> > +          // extra constants on these shim interfaces.
> > +          if (aSpec && idx == aIndex) {
> > +            *aSpec = cs;
> > +            return idx;
> 
> It is nice that you avoided the copy and paste stuff here, but it makes me
> uneasy how you are returning a value here that is never used, so it is
> untested code. Maybe make the two separate modes more explicit by returning
> the number of constants via a possibly-null argument, like it is done for
> aSpec? Then you can assert that only one or the other is being used, and
> thus there will be no untested calculations.

Sure

> @@ +134,5 @@
> > +    }
> > +
> > +    // Extract the value and name from the Constant Info.
> > +    const ConstInfo& info = sConsts[mConsts + aIndex];
> > +    if (info.mSigned || info.mValue <= (uint32_t)INT32_MAX) {
> 
> I'm not sure I 100% understand this but looking at the old code it seems
> reasonable.

The old code would save a nsXPTType for each constant, which was unnecessary as every constant is always int32_t, uint32_t, int16_t, or uint16_t. All of these types can be promoted to 32-bits, and then I save whether or not to interpret them as signed with int32_t.

Additionally, the JS engine can optimize when we have an int32_t by representing it as an int directly in the jsval, which ideally we want to use. We can always store signed values in an int32_t, and we can store every unsigned value which we can fit in an int32_t as well. We only need to use double for large uint32_t values.

> @@ +285,5 @@
> > +  return NS_ERROR_FAILURE;
> > +}
> > +
> > +nsresult
> > +nsXPTInterfaceInfo::GetTypeForParam(uint16_t aMethodIndex,
> 
> It feels strange that aMethodIndex is unused here and in the next few
> methods. I suppose that this is fallout from my earlier hoisting of
> parameter data to a global structure instead of a per-method structure.
> Maybe add a comment saying that it is unused? Technically your patch changes
> behavior in the case where somebody passes in an invalid index, but I don't
> think that's a real concern.

Yeah, that was my reasoning. I can add a comment noting that it is unused now due to the fact that all of this info is now global.

> ::: xpcom/reflect/xptinfo/xptinfo.h
> @@ +20,5 @@
> >  
> > +// Forward Declarations
> > +namespace mozilla {
> > +namespace dom {
> > +class NativePropertyHooks;
> 
> nit: this should be |struct|, not |class|. I get a lot of build warnings
> from this.

I have this fixed locally - oops.

> @@ +34,5 @@
> > +namespace detail {
> > +
> > +// xpt::detail::XPTConstruct is friend class with each of the data classes, such
> > +// that the generated code can access private fields.
> > +class XPTConstruct;
> 
> XPTConstruct is a struct here, but a class in xptdata.cpp. I'm getting a
> warning about that. This probably also applies to the friend class
> declarations below.

Yup, also fixed locally. oops D:

> @@ +42,5 @@
> > +inline const nsXPTParamInfo& GetParam(uint16_t aIdx);
> > +inline const nsXPTMethodInfo& GetMethod(uint16_t aIdx);
> > +inline const char* GetString(uint32_t aIdx);
> > +
> > +extern const uint16_t sInterfacesSize;
> 
> You declare this a second time down below, which seems a little weird. I
> guess you have to forward declare it because it appears in code in the
> header, but it would also be odd to not mention it down with the rest of the
> static variables...

I can remove the second declaration. I was just thinking about putting all of the static declarations in one place, but I also needed this one earlier *shrug*

> @@ +82,5 @@
> > +    return xpt::detail::GetInterface(mParent);
> > +  }
> > +
> > +  // Do we have an ancestor interface with the given IID?
> > +  bool HasAncestor(const nsIID& aIID) const {
> 
> OOC, why are you defining this and Method() inline?

Specifically in the case of Method() I was imagining that the optimizer could merge some of the checks from MethodCount() and Method() if it was inline, which I thought might be worthwhile. Basically it seemed small enough that I thought perhaps it would be worthwhile to inline.

(also I originally wrote this with everything inline except the XPCOM-style methods, and ended up pulling things out of line that were obviously a bit large to put in the header *shrug*)

> @@ +122,5 @@
> > +  // nsIInterfaceInfo backwards compatibility //
> > +  //////////////////////////////////////////////
> > +
> > +  nsresult GetName(char** aName) const;
> > +  nsresult GetInterfaceIID(nsIID** aIID) const;
> 
> I deleted this method, along with GetInfoForParam and GetIIDForParam, in bug
> 1449747, so you can remove them here.

nice!

I didn't really bother to check to see if these methods were used anywhere, I just implemented them so things would work. I can easily clean them up though ^_^

> @@ +129,5 @@
> > +  nsresult GetParent(const nsXPTInterfaceInfo** aParent) const;
> > +  nsresult GetMethodCount(uint16_t* aMethodCount) const;
> > +  nsresult GetConstantCount(uint16_t* aConstantCount) const;
> > +  nsresult GetMethodInfo(uint16_t aIndex, const nsXPTMethodInfo** aInfo) const;
> > +  nsresult GetMethodInfoForName(const char* aMethodName, uint16_t* aIndex,
> 
> GetMethodInfoForName and MethodByName are unused, so you should delete them
> rather than convert them. (I'm not sure when that happened. I'm pretty sure
> they were used a week or two ago!)

^see above

> @@ +159,5 @@
> > +                                 nsIID* aIID) const;
> > +  nsresult IsMainProcessScriptableOnly(bool* aRetval) const;
> > +
> > +  // XXX(nika): Remove?
> > +  bool EnsureResolved() const { return !mIsShim; }
> 
> Ah, good point. I didn't think about how shim interfaces act like they are
> unresolved.

Yeah, that being said I'm pretty sure that we should never see a shim interface where we call EnsureResolved(), so....

I'm not sure what the best option here is.

> @@ +164,5 @@
> > +
> > +private:
> > +  friend class xpt::detail::XPTConstruct;
> > +  constexpr nsXPTInterfaceInfo()
> > +    : mIID{0}, mName{0}, mParent{0}, mBuiltinClass{0}
> 
> Out of curiosity, why the {} instead of ()?

Uhh. I think there was a reason at one point, but it got refactored away. *shrug*.

> @@ +172,5 @@
> > +  /*
> > +   * This field ordering minimizes the size of this struct.
> > +   */
> > +  nsID mIID;
> > +  uint32_t mName; // Index into XPTHeader::mStrings.
> 
> Most or all of these "Index into" comments need to be updated. For instance,
> I think this is actually an index into xpt::detail::sStrings.

Yup, can do.

> @@ +176,5 @@
> > +  uint32_t mName; // Index into XPTHeader::mStrings.
> > +
> > +  uint16_t mParent : 14;
> > +  uint16_t mBuiltinClass : 1;
> > +  // XXX(nika): Do we need this if we don't have addons anymore?
> 
> Please file a bug about maybe removing this.

Ok

> @@ +191,3 @@
> >  };
> >  
> > +static_assert(sizeof(nsXPTInterfaceInfo) == 28, "wrong size?");
> 
> Please document what depends on this being that size, if anything. Likewise
> with the other static asserts.

Nothing depends on it. This was me checking that my packing worked, and I left it in to make sure that if someone changed the layout of one of these structs, they would have to consider how they were impacting the binary size.

> @@ +293,5 @@
> > +    return Tag() == TD_INTERFACE_IS_TYPE || Tag() == TD_ARRAY ||
> > +           Tag() == TD_PSTRING_SIZE_IS || Tag() == TD_PWSTRING_SIZE_IS;
> > +  }
> > +
> > +  bool IsSizedString() const {
> 
> I think this method is unused.

Oops - was part of an earlier patch.

> @@ +310,5 @@
> > +  constexpr nsXPTType()
> > +    : mTag{0}, mInParam{0}, mOutParam{0}, mOptionalParam{0}, mData1{0}, mData2{0}
> > +  { }
> > +  MOZ_IMPLICIT nsXPTType(const uint8_t& aPrefix)
> > +    : mTag(aPrefix) { MOZ_ASSERT(aPrefix <= TD_JSVAL); }
> 
> Should the other fields be initialized, too? Also, this uses () instead of
> {}, if that matters.

Yup. They should be. And it doesn't matter.

> @@ +370,5 @@
> > +static_assert(sizeof(nsXPTType) == 3, "wrong size");
> > +
> > +
> > +/*
> > + * A ParamDescriptor is used to describe either a single argument to a method or
> 
> nsXPTParamInfo, not ParamDescriptor.

I'll do a once over of all of the comments - it seems like I had a hayday for messing them up ^_^

> @@ +379,5 @@
> >  public:
> > +  bool IsIn() const { return mType.mInParam; }
> > +  bool IsOut() const { return mType.mOutParam && !IsDipper(); }
> > +  bool IsOptional() const { return mType.mOptionalParam; }
> > +  bool IsShared() const { return false; } // XXX remove (backcompat)
> 
> Should IsScriptable() above have the full "// XXX remove (backcompat)"
> comment? Or would it be harder to remove?

Yeah, we should dump it.

> @@ +398,5 @@
> > +  // parameters masquerading as in'. The burden of maintaining this illusion
> > +  // falls mostly on XPConnect, which creates the empty containers, and harvest
> > +  // the results after the call.
> > +  //
> > +  // Currently, the only dipper types are the string classes.
> 
> Ah, I see you moved the calculation of what is a dipper type from typelib.py
> to here. You should mention bug 677784 here, then.

Yeah, doing that work here saves me a bit on every parameter, which allowed me to compress from 4 bytes/param to 3 bytes/param ^_^

> @@ +514,5 @@
> > +extern const char sStrings[];
> > +extern const ConstInfo sConsts[];
> > +
> > +// shim constant information
> > +extern const mozilla::dom::NativePropertyHooks* sPropHooks[];
> 
> Just to double check here, this'll basically be about the same amount of
> relocatable data as we had with kComponentsInterfaceShimMap? (A little less,
> even!)

Yeah, it's not many relocations. I figured having ~20-something pointers in static memory was no biggie.


(In reply to Andrew McCreight [:mccr8] from comment #34)
> Cool! I'm glad this horrible interface is gone.

Me too! Seeing it in all of its awfulness TBH was one of the things which drove me into this madness (that and WebIDL)

> ::: js/xpconnect/src/XPCWrappedNative.cpp
> @@ +1399,5 @@
> >  CallMethodHelper::GetOutParamSource(uint8_t paramIndex, MutableHandleValue srcp) const
> >  {
> >      const nsXPTParamInfo& paramInfo = mMethodInfo->GetParam(paramIndex);
> >  
> > +    bool isRetval = mMethodInfo->HasRetval() &&
> 
> Please wrap this up into a predicate method on method info and use it
> everywhere. This seems a little finicky and hard to understand to have
> scattered around.

Sure.

(In reply to Andrew McCreight [:mccr8] from comment #32)
> This is a smaller amount, but I also see that the number of top-level types
> goes from about 40 to about 200 with your patch. I think there the issue is
> just that you are missing deduplication for types. For instance, uint8
> appears about 40 times.

Yup, as I mention above, I messed up my toplevel type deduplication code accidentally *sigh*

> This is a pre-existing issue, so I'd leave it for followup work unless you
> really need it, but I wonder if using a type index instead of an inline type
> in parameters would save space. The index is smaller, but maybe it wouldn't
> be if we needed a lot more top level types.

We'd need to probably bump up to at least 16 bits (2 bytes) for type index if we did that, given that there are ~1300 interfaces and we probably need types for a good fraction of them. In addition we'd have to figure out how to handle arrays, which still can only fit 8 bits of type index.

I think keeping the types inline in the param listings (where sizeof(nsXPTType) == sizeof(nsXPTParamInfo) due to my changes), is probably the better idea given that.
Thanks for the explanations. That all sounds good to me.

(In reply to Nika Layzell [:mystor] from comment #35)
> If we wanted to be as efficient as possible with PHF generation, we could
> use gperf rather than my code, but I don't think we want to add a new build
> dependency for Firefox right now, which is why I avoided doing so.
Yeah, that's the right call. I think the performance is good enough.

> The reasoning was that this data structure is in xpt::detail::* as it was
> internal, so I didn't give it the same naming style. I agree that it would
> probably be cleaner to toss that namespace & just call everything nsXPT*, so
> I'll probably do that.
Oh, I see. I did kind of like things being in xpt::detail:: because it makes it clear that you shouldn't look at it unless you are working on the internals. But whatever you think is best is fine.

> IIRC there were no test problems with this particular decision. There's
> nothing that XPConnect code can do with unresolved interfaces anyway, so...
Yeah, I don't know what the precise boundaries are for what is or isn't needed, but I assume that if something was wrong it would break everything horribly.

> Yeah, that being said I'm pretty sure that we should never see a shim
> interface where we call EnsureResolved(), so....
> 
> I'm not sure what the best option here is.

What you have is fine. Somebody can clean it up in followup work if they want.
One minor thing I meant to mention is that it looked like the upper half of every value in the hash stuff was zero, so maybe we could squeeze out a little bit of space that way? I don't know if it is needed or if that much space matters. Probably not.
(In reply to Andrew McCreight [:mccr8] from comment #37)
> One minor thing I meant to mention is that it looked like the upper half of
> every value in the hash stuff was zero, so maybe we could squeeze out a
> little bit of space that way? I don't know if it is needed or if that much
> space matters. Probably not.

In an earlier version of the code, I used a 16-bit value for the hash intermediates. It worked pretty well, but was too close for my comfort. I played around with adding/removing interfaces & managed to make it blow through the 16-bit maximum, so I figured I'd make it 32-bits to be safe.

For the larger table, I write the numbers out as though they were 32 bits wide, but they're actually stored as 16 bits. I should change the formatting string to make that more clear.
Attachment #8968296 - Flags: review?(continuation)
Attachment #8968300 - Flags: review?(continuation)
Attachment #8968296 - Flags: review?(continuation) → review+
I decided to just make a series of interdiffs to make it easier to review the changes.
Comment on attachment 8968297 [details] [diff] [review]
Intr 2: Move implicit_builtinclass into xpidl.py to effectively inherit from base interfaces

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

::: xpcom/idl-parser/xpidl/xpidl.py
@@ +581,5 @@
>          for m in members:
>              if not isinstance(m, CDATA):
>                  self.namemap.set(m)
>  
> +            if m.kind == 'method' and m.notxpcom:

How does this avoid tagging nsISupports as builtinclass?

::: xpcom/reflect/xptinfo/xptcodegen.py
@@ -298,5 @@
>              if idx == 0:
>                  break
>              iface = iid_phf.values[idx - 1]
>  
> -        return methods, consts, builtinclass

It looks like you are missing some changes here around the call site to collect_base_info.
Comment on attachment 8968298 [details] [diff] [review]
Intr 3: Clean up the logic for determining if a parameter is a retval

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

What about CallMethodHelper::GetOutParamSource() and CallMethodHelper::GatherAndConvertResults()?
What's the impact on the installer size on the OS of your choice with the new version of these patches? (Just asking you directly seems easier than trying to figure out how to apply these new patches locally, but I could work it out myself if you give me a rollup patch and a base commit or something.)
Attachment #8968296 - Attachment is obsolete: true
Attachment #8968297 - Attachment is obsolete: true
Attachment #8968298 - Attachment is obsolete: true
Attachment #8968297 - Flags: review?(continuation)
Attachment #8968298 - Flags: review?(continuation)
Attachment #8968322 - Flags: review?(continuation) → review+
Attachment #8968323 - Flags: review?(continuation) → review+
Comment on attachment 8968299 [details] [diff] [review]
Intr 4: Eliminate constexpr functions & cache params/types

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

::: xpcom/reflect/xptinfo/xptcodegen.py
@@ +252,2 @@
>  
> +            # If our method is hidden, we can save some memory by not

You can remove this comment now.

@@ +327,5 @@
> +        # The number of maximum methods is not arbitrary. It is the same value
> +        # as in xpcom/reflect/xptcall/genstubs.pl; do not change this value
> +        # without changing that one or you WILL see problems.
> +        #
> +        # In addition, mNumMethods and mNumConsts are stored as a 8-bit int,

nit: I think this should be "are stored as 8-bit ints".
Attachment #8968299 - Flags: review?(continuation) → review+
Attachment #8968300 - Flags: review?(continuation) → review+
Comment on attachment 8965864 [details] [diff] [review]
Part 4: Rewrite xptinfo, and write a new xptcodegen.py to generate the required datastructures

r=me with the changes in the interdiffs, assuming there's no big impact on the installer size from your patches.
Attachment #8965864 - Flags: review- → review+
Comment on attachment 8965864 [details] [diff] [review]
Part 4: Rewrite xptinfo, and write a new xptcodegen.py to generate the required datastructures

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

Mostly just nitpicky stuff, but f+ for now. I'd like to see some more comments / naming cleanup. The algorithm itself seems okay, but I'd like to see some tests for that. At some point it would be really nice to have a separate perfect hash table wrapper class that we could reuse (it would make adding tests easier as well) similar to DAFSA [1].

[1] https://searchfox.org/mozilla-central/search?q=symbol:T_mozilla%3A%3ADafsa&redirect=false

::: xpcom/reflect/xptinfo/phf.py
@@ +1,2 @@
> +#!/usr/bin/env python
> +# phf.py - Helper for generating perfect hash functions for xptcodegen.py

I think mystor already agreed, but perfecthash.py sounds good.

@@ +2,5 @@
> +# phf.py - Helper for generating perfect hash functions for xptcodegen.py
> +#
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

Can you provide a toplevel comment on what a FNV/perfect hash is and some useful links? Nothing huge, just make it easy for others to lookup. Also if this was inspired by any external code can you provide a link?

@@ +5,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +FNV_OFFSET_BASIS = 0x811C9DC5
> +FNV_PRIME = 16777619

Maybe make it explicit these are the 32-bit versions.

@@ +26,5 @@
> +
> +class PHF(object):
> +    """An object representing a perfect hash function"""
> +    def __init__(self, intsize, data):
> +        """Keys should be a list of (bytearray, value) pairs"""

I think you mean `data` instead of keys right? Can you document `intsize`? Actually now that I realized it's the intermediate table can you change the name to `intermediate_table_size` or something more explicit?

@@ +29,5 @@
> +    def __init__(self, intsize, data):
> +        """Keys should be a list of (bytearray, value) pairs"""
> +        mapsize = len(data) # Size of the target values array
> +
> +        buckets = [(i, []) for i in range(intsize)]

This is just a style nit, but I think it would make everything a bit easier to understand, right now were indexing into tuples like:

> buckets[hash(key) % intsize][1].append((key, val))
> buckets.sort(key=lambda b: len(b[1]), reverse=True)
> self.values[freecursor] = bucket[0][1]
> slot = hash(bucket[i][0], basis) % mapsize

Looking at that with fresh eyes is a bit confusing. I see a couple options:
  1a) Get rid of tracking the idx in buckets (so tuple[0]) and just recalculate it when inserting into the intermediate table
  1b) Do some sort of named tuple for this of (hash_idx, entries)
  2a) Add some sort of key/value named tuple for the entries
  2b) Just explode the tuple wherever it's used, ie: k,v = bucket

@@ +30,5 @@
> +        """Keys should be a list of (bytearray, value) pairs"""
> +        mapsize = len(data) # Size of the target values array
> +
> +        buckets = [(i, []) for i in range(intsize)]
> +        self.inter = [0] * intsize

It's more verbose, but maybe intermediate would be more appropriate here. Well unless you mean some variation of inter.

@@ +45,5 @@
> +        # Look at the largest bucket first.
> +        buckets.sort(key=lambda b: len(b[1]), reverse=True)
> +
> +        freecursor = 0
> +        for idx, bucket in buckets:

nit: maybe `intermediate_idx`, or if you take suggestion 1a above this goes away.

::: xpcom/reflect/xptinfo/xptcodegen.py
@@ +28,5 @@
> +    assert idx == len(s)
> +
> +def split_iid(iid): # Get the individual components out of an IID string.
> +    iid = iid.replace('-', '') # Strip any '-' delimiters
> +    return tuple(split_at_idxs(iid, (8, 4, 4, 2, 2, 2, 2, 2, 2, 2, 2)))

I think a high level doc about why this is tedious and short description of our crazytown internal nsID format is worth noting here.

@@ +32,5 @@
> +    return tuple(split_at_idxs(iid, (8, 4, 4, 2, 2, 2, 2, 2, 2, 2, 2)))
> +
> +def iid_bytes(iid): # Get the byte representation of the IID for hashing.
> +    bs = bytearray()
> +    # We store the bytes in little-endian (XXX: Big endian support?)

If we supported it before, then yes we need it.

TBH I'm surprised nsID isn't just always using a bigendian array, it would be much easier to encode / decode / compare. We have to work with what we have I guess :(

::: xpcom/reflect/xptinfo/xptinfo.cpp
@@ +22,5 @@
> +static const uint32_t FNV_PRIME = 16777619;
> +static const uint32_t U32_HIGH_BIT = 0x80000000;
> +
> +static uint32_t
> +Phf_DoHash(const void* bytes, uint32_t len, uint32_t h=FNV_OFFSET_BASIS)

At some point it would be nice to move this out into a separate class, similar to our DAFSA impl so that it can be reused.

@@ +50,5 @@
> +
> +/* static */ const nsXPTInterfaceInfo*
> +nsXPTInterfaceInfo::ByIID(const nsIID& aIID)
> +{
> +  uint16_t idx = Phf_DoLookup(&aIID, sizeof(nsIID), sPHF_IIDs);

We probably need to assert this index is in range.

@@ +52,5 @@
> +nsXPTInterfaceInfo::ByIID(const nsIID& aIID)
> +{
> +  uint16_t idx = Phf_DoLookup(&aIID, sizeof(nsIID), sPHF_IIDs);
> +
> +  const nsXPTInterfaceInfo* found = &sInterfaces[idx];

In what instance do we not find the interface? For the perfect hash to work every lookup needs to be valid

@@ +60,5 @@
> +
> +/* static */ const nsXPTInterfaceInfo*
> +nsXPTInterfaceInfo::ByName(const char* aName)
> +{
> +  uint16_t idx = Phf_DoLookup(aName, strlen(aName), sPHF_Names);

Ditto.

::: xpcom/reflect/xptinfo/xptinfo.h
@@ +517,5 @@
> +// shim constant information
> +extern const mozilla::dom::NativePropertyHooks* sPropHooks[];
> +
> +// Perfect Hash Function backing data
> +static const uint16_t kPHFSize = 256;

Often a prime number is used (particularly for smaller tables) as it gets better distribution. I get that this is a nice mod value but we've found that is still a net loss in the past. It would be interesting to at least *test* a different value.
Attachment #8965864 - Flags: review?(erahm)
Attachment #8965864 - Flags: review-
Attachment #8965864 - Flags: review+
Attachment #8965864 - Flags: feedback+
(In reply to Andrew McCreight [:mccr8] from comment #55)
> r=me with the changes in the interdiffs, assuming there's no big impact on
> the installer size from your patches.

From my quick test, this caused a reduction of 3.5k bytes in terms of installer size. (NOTE: This includes the webidl patches too)
(In reply to Nika Layzell [:mystor] from comment #57)
> From my quick test, this caused a reduction of 3.5k bytes in terms of
> installer size. (NOTE: This includes the webidl patches too)

Great! Thanks for checking.
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #56)
> Mostly just nitpicky stuff, but f+ for now. I'd like to see some more
> comments / naming cleanup. The algorithm itself seems okay, but I'd like to
> see some tests for that. At some point it would be really nice to have a
> separate perfect hash table wrapper class that we could reuse (it would make
> adding tests easier as well) similar to DAFSA [1].
> 
> [1]
> https://searchfox.org/mozilla-central/search?q=symbol:
> T_mozilla%3A%3ADafsa&redirect=false

I would like to do the generic-ification as a follow-up, rather than doing it in this bug. The bug's gotten big enough as-is.

> ::: xpcom/reflect/xptinfo/phf.py
> @@ +1,2 @@
> > +#!/usr/bin/env python
> > +# phf.py - Helper for generating perfect hash functions for xptcodegen.py
> 
> I think mystor already agreed, but perfecthash.py sounds good.

Already done :-). Works for me.

> @@ +2,5 @@
> > +# phf.py - Helper for generating perfect hash functions for xptcodegen.py
> > +#
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> Can you provide a toplevel comment on what a FNV/perfect hash is and some
> useful links? Nothing huge, just make it easy for others to lookup. Also if
> this was inspired by any external code can you provide a link?

Sure. I can do this.

> @@ +5,5 @@
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +FNV_OFFSET_BASIS = 0x811C9DC5
> > +FNV_PRIME = 16777619
> 
> Maybe make it explicit these are the 32-bit versions.

OK.

> @@ +26,5 @@
> > +
> > +class PHF(object):
> > +    """An object representing a perfect hash function"""
> > +    def __init__(self, intsize, data):
> > +        """Keys should be a list of (bytearray, value) pairs"""
> 
> I think you mean `data` instead of keys right? Can you document `intsize`?
> Actually now that I realized it's the intermediate table can you change the
> name to `intermediate_table_size` or something more explicit?

Yes.

> @@ +29,5 @@
> > +    def __init__(self, intsize, data):
> > +        """Keys should be a list of (bytearray, value) pairs"""
> > +        mapsize = len(data) # Size of the target values array
> > +
> > +        buckets = [(i, []) for i in range(intsize)]
> 
> This is just a style nit, but I think it would make everything a bit easier
> to understand, right now were indexing into tuples like:

I think you failed to finish this comment?

I might make this a namedtuple so we can avoid the array indexing.

> > buckets[hash(key) % intsize][1].append((key, val))
> > buckets.sort(key=lambda b: len(b[1]), reverse=True)
> > self.values[freecursor] = bucket[0][1]
> > slot = hash(bucket[i][0], basis) % mapsize
> 
> Looking at that with fresh eyes is a bit confusing. I see a couple options:
>   1a) Get rid of tracking the idx in buckets (so tuple[0]) and just
> recalculate it when inserting into the intermediate table
>   1b) Do some sort of named tuple for this of (hash_idx, entries)
>   2a) Add some sort of key/value named tuple for the entries
>   2b) Just explode the tuple wherever it's used, ie: k,v = bucket

I think the namedtuple option would work well for clearing this up, so I think I will take that option.

> @@ +30,5 @@
> > +        """Keys should be a list of (bytearray, value) pairs"""
> > +        mapsize = len(data) # Size of the target values array
> > +
> > +        buckets = [(i, []) for i in range(intsize)]
> > +        self.inter = [0] * intsize
> 
> It's more verbose, but maybe intermediate would be more appropriate here.
> Well unless you mean some variation of inter.

Sure.

> ::: xpcom/reflect/xptinfo/xptcodegen.py
> @@ +28,5 @@
> > +    assert idx == len(s)
> > +
> > +def split_iid(iid): # Get the individual components out of an IID string.
> > +    iid = iid.replace('-', '') # Strip any '-' delimiters
> > +    return tuple(split_at_idxs(iid, (8, 4, 4, 2, 2, 2, 2, 2, 2, 2, 2)))
> 
> I think a high level doc about why this is tedious and short description of
> our crazytown internal nsID format is worth noting here.

Yup. I can do that.

> @@ +32,5 @@
> > +    return tuple(split_at_idxs(iid, (8, 4, 4, 2, 2, 2, 2, 2, 2, 2, 2)))
> > +
> > +def iid_bytes(iid): # Get the byte representation of the IID for hashing.
> > +    bs = bytearray()
> > +    # We store the bytes in little-endian (XXX: Big endian support?)
> 
> If we supported it before, then yes we need it.
> 
> TBH I'm surprised nsID isn't just always using a bigendian array, it would
> be much easier to encode / decode / compare. We have to work with what we
> have I guess :(

Yeah, that would be nice. I could change the representation to be consistently big-endian in gecko, but that feels like it might be a bit of a larger project to ensure works well. Is there a good way for me to check in this script whether or not the target architecture is big-endian?

> @@ +52,5 @@
> > +nsXPTInterfaceInfo::ByIID(const nsIID& aIID)
> > +{
> > +  uint16_t idx = Phf_DoLookup(&aIID, sizeof(nsIID), sPHF_IIDs);
> > +
> > +  const nsXPTInterfaceInfo* found = &sInterfaces[idx];
> 
> In what instance do we not find the interface? For the perfect hash to work
> every lookup needs to be valid.

If you try to look up an IID which isn't scriptable, or one which doesn't exist, then you'll perform a lookup and get a random interface. This will only happen if the IID isn't in the table, so we just do an equality check of the IID/name and return nullptr if we failed to perform the lookup.

Every entry which is in the table will take you to the right entry, so I'm pretty sure this is sound.

> ::: xpcom/reflect/xptinfo/xptinfo.h
> @@ +517,5 @@
> > +// shim constant information
> > +extern const mozilla::dom::NativePropertyHooks* sPropHooks[];
> > +
> > +// Perfect Hash Function backing data
> > +static const uint16_t kPHFSize = 256;
> 
> Often a prime number is used (particularly for smaller tables) as it gets
> better distribution. I get that this is a nice mod value but we've found
> that is still a net loss in the past. It would be interesting to at least
> *test* a different value.

My reasoning here is that getting better distribution isn't that big of a deal because the hash values are precomputed, so the distribution quality doesn't really matter as much. I think we can get away with it.
Blocks: 1451959
Comment on attachment 8965864 [details] [diff] [review]
Part 4: Rewrite xptinfo, and write a new xptcodegen.py to generate the required datastructures

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

glandium, in xptcodegen.py we need to know the endianeness of the target platform when generating some hashtables. Can you help us find a way to hook that into the build system?
Attachment #8965864 - Flags: feedback?(mh+mozilla)
(In reply to Nika Layzell [:mystor] from comment #59)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #56)
> > Mostly just nitpicky stuff, but f+ for now. I'd like to see some more
> > comments / naming cleanup. The algorithm itself seems okay, but I'd like to
> > see some tests for that. At some point it would be really nice to have a
> > separate perfect hash table wrapper class that we could reuse (it would make
> > adding tests easier as well) similar to DAFSA [1].
> > 
> > [1]
> > https://searchfox.org/mozilla-central/search?q=symbol:
> > T_mozilla%3A%3ADafsa&redirect=false
> 
> I would like to do the generic-ification as a follow-up, rather than doing
> it in this bug. The bug's gotten big enough as-is.

For sure, I just wanted you to know that I think this is valuable elsewhere :)


> > This is just a style nit, but I think it would make everything a bit easier
> > to understand, right now were indexing into tuples like:
> 
> I think you failed to finish this comment?

Yeah bad editing, I just wanted to justify not using tuples for the sake of code sanity.
 
> Yeah, that would be nice. I could change the representation to be
> consistently big-endian in gecko, but that feels like it might be a bit of a
> larger project to ensure works well. Is there a good way for me to check in
> this script whether or not the target architecture is big-endian?

That would definitely be a follow up (not necessarily for you). I ni'd glandium to see if he has any ideas re: endianness. Another option would be to have the C++ side flip the bits appropriately when hashing on big endian which is janky but better than broken.

> > @@ +52,5 @@
> > > +nsXPTInterfaceInfo::ByIID(const nsIID& aIID)
> > > +{
> > > +  uint16_t idx = Phf_DoLookup(&aIID, sizeof(nsIID), sPHF_IIDs);
> > > +
> > > +  const nsXPTInterfaceInfo* found = &sInterfaces[idx];
> > 
> > In what instance do we not find the interface? For the perfect hash to work
> > every lookup needs to be valid.
> 
> If you try to look up an IID which isn't scriptable, or one which doesn't
> exist, then you'll perform a lookup and get a random interface. This will
> only happen if the IID isn't in the table, so we just do an equality check
> of the IID/name and return nullptr if we failed to perform the lookup.
> 
> Every entry which is in the table will take you to the right entry, so I'm
> pretty sure this is sound.

Sorry I completely missed the equality check.

> > ::: xpcom/reflect/xptinfo/xptinfo.h
> > @@ +517,5 @@
> > > +// shim constant information
> > > +extern const mozilla::dom::NativePropertyHooks* sPropHooks[];
> > > +
> > > +// Perfect Hash Function backing data
> > > +static const uint16_t kPHFSize = 256;
> > 
> > Often a prime number is used (particularly for smaller tables) as it gets
> > better distribution. I get that this is a nice mod value but we've found
> > that is still a net loss in the past. It would be interesting to at least
> > *test* a different value.
> 
> My reasoning here is that getting better distribution isn't that big of a
> deal because the hash values are precomputed, so the distribution quality
> doesn't really matter as much. I think we can get away with it.

Fair point.
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #61)
> > Yeah, that would be nice. I could change the representation to be
> > consistently big-endian in gecko, but that feels like it might be a bit of a
> > larger project to ensure works well. Is there a good way for me to check in
> > this script whether or not the target architecture is big-endian?
> 
> That would definitely be a follow up (not necessarily for you). I ni'd
> glandium to see if he has any ideas re: endianness. Another option would be
> to have the C++ side flip the bits appropriately when hashing on big endian
> which is janky but better than broken.

I think I'll do that for now, and we can come back & take a better approach in the future if we discover a better way to handle this.
Here are some changes in reply to your patches.
Attachment #8968369 - Flags: review?(erahm)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #60)
> Comment on attachment 8965864 [details] [diff] [review]
> Part 4: Rewrite xptinfo, and write a new xptcodegen.py to generate the
> required datastructures
> 
> Review of attachment 8965864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> glandium, in xptcodegen.py we need to know the endianeness of the target
> platform when generating some hashtables. Can you help us find a way to hook
> that into the build system?

Expose the endianness through a set_config in build/moz.configure/init.configure, and get the value from buildconfig.substs in the code gen python script.
Flags: needinfo?(mh+mozilla)
Attachment #8965864 - Flags: feedback?(mh+mozilla)
Comment on attachment 8968369 [details] [diff] [review]
Intr 6: Changes to Perfect Hashing

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

Thanks for the interdiff, lgtm.
Attachment #8968369 - Flags: review?(erahm) → review+
(In reply to Mike Hommey [:glandium] from comment #64)
> Expose the endianness through a set_config in
> build/moz.configure/init.configure, and get the value from
> buildconfig.substs in the code gen python script.

Currently xptcodegen.py isn't part of the virtualenv (like the other xpidl stuff) IIRC, so I'm not sure whether or not buildconfig is available there?

In any case, I think this can be done in a follow-up.
I think you can add -I $(DEPTH)/build to the PYTHONPATH call wrapping xpidl stuff and import buildconfig.
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da98166e73a8
Part 1: Clear out xptinfo and typelib to make way for the this patch, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6049a77ddc2
Part 2: Add the xptshim and xptshimfile attributes to xpidl, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0b17dbda47e
Part 3: Replace the XPT file format with a JSON based one, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4425a51bc165
Part 4: Rewrite xptinfo, and write a new xptcodegen.py to generate the required datastructures, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/90e79480ebc6
Part 5: Update consumers of nsIInterfaceInfo to use the nsXPTInterfaceInfo directly, r=mccr8
Blocks: 1455024
Depends on: 1455221
See Also: → 1769588
You need to log in before you can comment on or make changes to this bug.