Of the 0.83MB of data in every process for xpti-working-set, about 200KB of it is strings. I have a hacky patch that eliminates the dynamically allocated strings entirely by assuming the existence of a giant string that contains all of the strings mashed together, that looks like this: const char* const xpt_strings = "abandonBFCacheEntry\0" "abort\0" "ABORT\0" [...] Then, instead of using char* for XPTI strings, it uses integers that are an offset into this array. I'm assuming that with the right combination of modifiers on that array, it will be shared between all processes. My patch currently does not actually emit this string (I wrote my own script to emit it), and also the offsets should be calculated ahead of time (my current patch dynamically searches the entire string to find a desired string, which is obviously terrible...).

This won't apply to m-c, because it is on top of some other cleanups I have, but it'll give you the gist of it.

I'm working on generating the string table, but the linking of all XPT files into a single file doesn't happen as part of the basic mach build. Presumably this is only done when you build the final package. I'm not sure if there's any way to make this work with a static strings table besides always linking the XPT files into a single file. This will presumably make builds that touch a single .idl file slower but maybe that's okay.

It seems like a mixed approach, that still uses XPT files but statically allocates some data like strings, won't be easier than removing XPT files entirely and instead using code generation for the data, so I've been looking at that.

This will include the stuff in xpt_struct.h, but not the other data structures in reflect. That's most but not all of the memory under xpti-working-set.

This builds, for whatever that is worth, but it is missing generation of a top-level registration method for the XPT data structures. It eliminates XPT files, though I haven't purged them out of all of the build/package stuff. MozReview-Commit-ID: BtGu1qDVDWj

The basic idea is to eliminate all pointers from the data structures in xpt_struct.h (except XPTHeader) by replacing them with indices into per-XPTI module arrays (so the data can be shared), then have xpt.py generate C++ code representing those data structures instead of an XPT file. I think this ends up requiring an extra load, but hopefully none of these methods are hot enough for that to matter. If all XPT data was put into a single module (like I was basically doing in my prior prototype) then I think you could avoid that, at the cost of losing separate compilation of XPIDL files, which sounds like a bad tradeoff.

Updated a little with what the generated top level registration code might look like. I had to constify a lot of things in xpt_struct.h so that it would actually compile.

With this patch, I have something that at least starts up the browser. (It can't be landed as is, as it requires a little bit of manual build work, as outlined in register.py.) xpti-working-set is 0.22 MB with the patch. It looks like half of that is the stuff owned by XPTInterfaceInfoManager (which is hash tables mapping IIDs and strings to xptiInterfaceEntry*), and the other half is the remaining things stored in gXPTIStructArena (which is xptiTypelibGuts and xptiInterfaceEntry, which are just a thin wrapper around the XPT data so maybe we could get rid of that). The hash tables could theoretically be done statically, too.

Here's a new version of the patch. The main improvement is that it no longer requires manual build steps, though the build system part of this is still horrible and unlandable, probably. I also removed everything I could find from the packaging process related to XPT files. The way my patch works now is that the first phase of building XPT files for the various modules is the same. However, XPT files are now just an intermediate format for the build process. Then, as part of the regular build process, we call XPT link to link all of the XPT files together, like we'd normally do during packaging, except that the output is a giant C++ file instead of another XPT file. This needs to be done when any .idl file is changed, but with bug 1443230 in place, it takes less than a second, so that doesn't seem too unreasonable. I used njn's script from bug 1254777 to confirm that these tables are rodata on Linux. This seems to pass try, which is good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f0352e95cbe9773e2e0d20b48bad0a7a4dd9dd6

I realized that because my latest version was only generating a single XPTHeader that I could make that header a const global variable so that I didn't have to pass it around anywhere. This makes my patch much smaller, allowing me to avoid having to figure out the answer to questions like "Do I really need to change all 30 or more stubs files?". I think the patch is in a good state, aside from the build system integration.

This is basically the same as the previous version, but seeing it split up into the individual changes with comments may make it easier to understand.

I checked an opt build on Linux, with and without my patches, for the size of firefox-60.0a1.en-US.linux-x86_64.tar.bz2. Without my patches, the size is 69413915. With them, it is 69451188. So, my patches as-is make things 37273 bytes larger.

I tried changing XPTMethodDescriptor::mParams from a uint32_t to a uint16_t, but that made the installer larger. Very odd. I'll just leave it as is and leave twiddling at that level to somebody better informed on these issues. I also tried merging together XPTInterfaceDirectoryEntry and XPTInterfaceDescriptor. This adds a few fields to each unresolved interface, but makes the amount of space needed for each resolved interface by one 32 bit int. In a build with a single XPT, there are 40 of the former and 1200 of the latter, so it is a good tradeoff. With this change, the installer was slightly smaller (about 5000 bytes less) than without my changes. So hopefully installer size won't be a blocker for this.

I implemented a suggestion Nathan had about using a suffix tree to reduce the length of the strings. It saved 1159 bytes of strings, which isn't terrible, but I'm going to skip that bit of complexity for now. For instance, there is a constant LOAD_FLAGS_FORCE_ALLOW_DATA_URI which can be stored entirely within INTERNAL_LOAD_FLAGS_FORCE_ALLOW_DATA_URI.

I took a look at your patches last night, & they're already looking awesome :-). Are you planning to keep the .xpt format around during the build step? It seems like a lot of now-unnecessary complexity to have these build intermediates be in a custom binary format rather than just dumping out the relevant information into a .json, and reading those in to link.

(In reply to Nika Layzell [:mystor] from comment #28) > I took a look at your patches last night, & they're already looking awesome > :-). Thanks! > Are you planning to keep the .xpt format around during the build step? It > seems like a lot of now-unnecessary complexity to have these build > intermediates be in a custom binary format rather than just dumping out the > relevant information into a .json, and reading those in to link. Yeah, I thought about that. Long term, I think it makes sense to do that (for instance, there's a comment somewhere that they can't use some technique to avoid writing a file if it didn't change because XPT files are binary), but I'm not going to do that in this bug to reduce the complexity of the patch. I should file a bug, though.

I did an all-platform try push. Everything is green, except Windows which does not build. Specifically, MSVC does not like my autogenerated file. I think the first problem is that it turns out the syntax I was using for initializing unions (".foo = 12345") is C99, not C++, so Clang and GCC support it but MSVC does not. It looks like I failed to notice that the page on cppreference.com that I got when I googled for "union initialization" was for C and not C++. Hopefully there's some way to work around that. Maybe I can define a bunch of constexpr constructors. The second issue is that MSVC does not like my 164,000 character string literal. 65535 bytes is the limit. I may be able to work around that by splitting up the different types of strings (eg interface names, method names, etc.). That can probably be done without increasing the total size of strings by too much. Failing that, I can segment the string array, and store string references as a segment plus offset.

(In reply to Andrew McCreight [:mccr8] from comment #30) > The second issue is that MSVC does not like my 164,000 character string > literal. 65535 bytes is the limit. I may be able to work around that by > splitting up the different types of strings (eg interface names, method > names, etc.). That can probably be done without increasing the total size of > strings by too much. Failing that, I can segment the string array, and store > string references as a segment plus offset. For other cases where we have large autogenerated strings, the thing I've done is: const char String[] = { 'a', 'b', 'c', ... }; which is exactly as terrible as you would imagine, but it stops MSVC from complaining, and you don't have to do any weird workarounds.

My fix to the MSVC issues has two parts, for the two unions. For XPTConstValue, I created a constexpr ctor for each case. I resolved the ambiguity by doing a static cast in the autogenerated code at every call site. (My first attempt was to define a method for each case to create and return a union of the appropriate kind, but this caused an internal compiler error in GCC 5.4.) For XPTTypeDescriptor, I eliminated the union, in bug 1447849. I tried the constexpr ctor approach, but I was getting a lot of errors about deleted default constructor or something, and removing the union I disliked anyways seemed easier than figuring out what that meant. I verified locally (in a debug build, which hopefully does not matter) that things are still in rodata with these changes. This builds and passes tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0db5c94a37354331d72416056cabf062da030486 I'm going to do some local testing of a few build dependency changes, and do another all platform push. Then hopefully this will be ready for review.

Here's a sample of what the generated file looks like. The [...] indicates places where I cut stuff out. The full file is 26,673 lines long and 1,716,816 bytes. I've started printing each array element on its own line because putting the entire thing on a single line seemed to make Emacs not show line numbers any more, and it was hard to navigate the file.

On my local machine with gcc 5.4 in an opt linux64 build, without my patches firefox-61.0a1.en-US.linux-x86_64.tar.bz2 is 70348095 bytes. With it, it is 70283936. So, my patch reduces the installer size there by about 64000 bytes. I used njn's script to confirm that in an opt build, sInterfaces, sTypes, sParams, sMethods, sConsts, and sStrings are all rodata. The entry for kHeader is ".data.rel.ro.local 0000000000000038 .hidden XPTHeader::kHeader", but it is expected that kHeader (which is a small struct) is going to need to be relocatable. I did a full platform try push and it was all green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc4819c45b41d1bb86c81d8942c4a69a3e59ecbb&filter-tier=1

Glandium, for part 6, I'd like you to review the build system changes. Of course, feel free to also review the rest of it if you want.

Comment on attachment 8957574 [details] Bug 1438688, part 1 - Add methods for accessing arrays in xpt_struct.h. https://reviewboard.mozilla.org/r/226454/#review235976 I'm not sure if this makes sense w.r.t. the current or future code, but: is it worth making these fields private now that we have getters for them?

Comment on attachment 8957575 [details] Bug 1438688, part 2 - Load XPT information from a static variable instead of a file. https://reviewboard.mozilla.org/r/226456/#review236004 Lots of removed code, nice. ::: commit-message-00351:12 (Diff revision 3) > + > +XPT information is now loaded directly from a single static data > +structure, XPTHeader::kHeader, which will be automatically generated > +at compile time from .idl files (via .xpt files). Note that the script > +to do that is not added until part 6 of this patch series, so linking > +will fail on parts 2 through 5. That's unfortunate, because it could cause problems for people bisecting. Are you planning to roll these patches up before landing?

Comment on attachment 8957577 [details] Bug 1438688, part 4 - Hoist arrays to XPTHeader. https://reviewboard.mozilla.org/r/226460/#review236008 ::: xpcom/typelib/xpt/xpt_struct.h:54 (Diff revision 3) > inline const XPTInterfaceDescriptor* InterfaceDescriptor() const; > inline const char* Name() const; > > nsID mIID; > - const char* mName; > - const XPTInterfaceDescriptor* mInterfaceDescriptor; > + uint32_t mName; // Index into XPTHeader::mStrings. > + uint32_t mInterfaceDescriptor; // 1-based index into XPTHeader::mInterfaces. Why is it 1-based?

Comment on attachment 8957577 [details] Bug 1438688, part 4 - Hoist arrays to XPTHeader. https://reviewboard.mozilla.org/r/226460/#review236010

Comment on attachment 8957577 [details] Bug 1438688, part 4 - Hoist arrays to XPTHeader. https://reviewboard.mozilla.org/r/226460/#review236012

Comment on attachment 8957578 [details] Bug 1438688, part 5 - Merge XPTInterfaceDirectoryEntry and XPTInterfaceDescriptor. https://reviewboard.mozilla.org/r/226462/#review236014

Comment on attachment 8959383 [details] Bug 1438688, part 6 - Compile XPT information to C++ at build time. https://reviewboard.mozilla.org/r/228210/#review236016 ::: xpcom/typelib/xpt/tools/xpt.py:37 (Diff revision 2) > > """ > A module for working with XPCOM Type Libraries. > > The XPCOM Type Library File Format is described at: > -http://www.mozilla.org/scriptable/typelib_file.html . It is used > +https://www-archive.mozilla.org/scriptable/typelib_file.html . It Nit: no space before the '.'. ::: xpcom/typelib/xpt/tools/xpt.py:139 (Diff revision 2) > return len(self._list) > > > +class CodeGenData(object): > + """ > + This stores the top-level data needed to code gen XPT information in C++. Nit: "code gen" is a strange-sounding verb. Use "generate" instead? ::: xpcom/typelib/xpt/tools/xpt.py:142 (Diff revision 2) > +class CodeGenData(object): > + """ > + This stores the top-level data needed to code gen XPT information in C++. > + |methods| and |constants| are the top-level declarations in the module. > + These contain names, and so are not likely to benefit from deduplication. > + |params| are the lists of parameters for |methods|, stored concanated. Nit: concatenated ::: xpcom/typelib/xpt/tools/xpt.py:143 (Diff revision 2) > + """ > + This stores the top-level data needed to code gen XPT information in C++. > + |methods| and |constants| are the top-level declarations in the module. > + These contain names, and so are not likely to benefit from deduplication. > + |params| are the lists of parameters for |methods|, stored concanated. > + These are deduplicated if there are only a few. |types| and |strings| are "if there are only a few" -- really? That seems strange. ::: xpcom/typelib/xpt/tools/xpt.py:158 (Diff revision 2) > + > + self.params = [] > + self.params_indexes = {} > + > + self.methods = [] > + self.constants = [] Nit: blank line between methods and constants? ::: xpcom/typelib/xpt/tools/xpt.py:165 (Diff revision 2) > + self.strings = [] > + self.string_indexes = {} > + self.curr_string_index = 0 > + > + @staticmethod > + def write_array_body(fd, l): Nit: I initially thought this parameter was `1` (one) rather than `l` (ell). Use a different name? ::: xpcom/typelib/xpt/tools/xpt.py:220 (Diff revision 2) > + if len(new_params) == 0: > + return 0 > + > + index = len(self.params) > + # The limit of 4 here is fairly arbitrary. The idea is to not > + # spend add too many large things to the cache that have Nit: "not spend add" needs rewording. ::: xpcom/typelib/xpt/tools/xpt.py:1504 (Diff revision 2) > + def code_gen_iid(iid): > + chunks = iid.split('-') > + return "{0x%s, 0x%s, 0x%s, {0x%02x, 0x%02x, 0x%02x, 0x%02x, 0x%02x, 0x%02x, 0x%02x, 0x%02x}}" % ( > + chunks[0], chunks[1], chunks[2], > + int(chunks[3][:2], 16), int(chunks[3][2:4], 16), > + int(chunks[4][:2], 16), int(chunks[4][2:4], 16), I'd probably write [0:2] rather than [:2] for consistency with the other slices.

There is lots of output like this: > static const XPTTypeDescriptor sTypes[] = { > {{0x91}, 0, 0}, > {{0x8}, 0, 0}, > {{0x90}, 0, 0}, > {{0x4}, 0, 0}, > {{0x92}, 0, 14}, > [...] Is it possible/sensible to have any comments indicating something about where the values come from? I imagine that would make debugging this output much easier...

Comment on attachment 8957576 [details] Bug 1438688, part 3 - Remove XPT files from the packaging process. https://reviewboard.mozilla.org/r/226458/#review236032 I'm not sure the packaging code is not used to unpack older versions of Firefox, and in that case, this change would break that. It would be safer to keep the packaging code. I don't think people can accidentally add xpt files if nothing produces them anymore.

Comment on attachment 8959383 [details] Bug 1438688, part 6 - Compile XPT information to C++ at build time. https://reviewboard.mozilla.org/r/228210/#review236034 I'll have to take a deeper dive into the generator code and look at the generated code, but in the meanwhile, here's a quick note: ::: config/makefiles/xpidl/Makefile.in:68 (Diff revision 2) > +$(generated_file): $(xpt_files) $(code_gen_py) > + $(REPORT_BUILD) > + $(PYTHON_PATH) $(PLY_INCLUDE) \ > + $(code_gen_py) linkgen \ > + $(generated_file) $(xpt_files) Gah, so you still generate xpt files... you'll want to make them never end up in dist/bin, which I don't think you're doing here.

(In reply to Mike Hommey [:glandium] from comment #51) > Gah, so you still generate xpt files... you'll want to make them never end > up in dist/bin, which I don't think you're doing here. I think the plan is to make xpt files go away completely in bug 1447337.

Another question: what's the effect on the "xpti-working-set" value in about:memory? Does it get rid of that entirely? Is the memory reporter still required?

(In reply to Nicholas Nethercote [:njn] from comment #42) > I'm not sure if this makes sense w.r.t. the current or future code, but: is > it worth making these fields private now that we have getters for them? Conceptually, they are private, but it seems like they need to be public in order to initialize them from a list. For instance, if I make XPTInterfaceDescriptor::mFlags private, I get a lot of errors that look like this: XPTInfo.cpp:1238:1: error: could not convert '{{4288658647u, 38167, 17204, {185, 122, 206, 237, 120, 144, 153, 116}}, 153991, 7976, 2432, 36, 7, 0, 128}' from '<brace-enclosed initializer list>' to 'const XPTInterfaceDescriptor' I agree that the current state is not great, though it isn't too likely anybody will mess with it. I think I could make them private if I define constexpr ctors and use those to do the initialization. Do you want me to do that in here, or can I leave that to a followup? (In reply to Nicholas Nethercote [:njn] from comment #43) > That's unfortunate, because it could cause problems for people bisecting. > Are you planning to roll these patches up before landing? (In reply to Nicholas Nethercote [:njn] from comment #44) > Why is it 1-based? I forgot to explain this because I actually delete this in part 5. :) I'll add a comment about it. Zero is needed for unresolved interfaces. Before this patch, mInterfaceDescriptor would be a null pointer for unresolved interfaces. (In part 5, I change to checking that the ID is all zero.) (In reply to Nicholas Nethercote [:njn] from comment #48) > > The XPCOM Type Library File Format is described at: > > -http://www.mozilla.org/scriptable/typelib_file.html . It is used > > +https://www-archive.mozilla.org/scriptable/typelib_file.html . It > > Nit: no space before the '.'. I think this is intentional so that it is easier to copy and paste the URL. I can change it to The XPCOM Type Library File Format is described at: http://www.mozilla.org/scriptable/typelib_file.html It is used [...] So the URL is still nice and the weird space period is not there. > Nit: "code gen" is a strange-sounding verb. Use "generate" instead? Fixed. > Nit: concatenated Fixed. I don't know why I always have such trouble spelling that word. > "if there are only a few" -- really? That seems strange. This was mostly me being worried about perf issues for methods with lots of parameters, but being too lazy to actually see if it was a problem. I could check dropping that causes any real impact on the speed if you want. > Nit: blank line between methods and constants? Fixed. > Nit: I initially thought this parameter was `1` (one) rather than `l` (ell). > Use a different name? Good point. Too much functional programming on my brain. I'll name it "iterator" I guess. > Nit: "not spend add" needs rewording. Fixed. > I'd probably write [0:2] rather than [:2] for consistency with the other > slices. Good point. Fixed. (In reply to Nicholas Nethercote [:njn] from comment #49) > Is it possible/sensible to have any comments indicating something about > where the values come from? I imagine that would make debugging this output > much easier... Are you thinking of a comment at the top of the definition of each variable that explains what each field is, or a comment on every line explaining the value? The latter seems kind of bloated to me, but I could try using the existing method that converts each data structure to a string.

(In reply to Nika Layzell [:mystor] from comment #52) > I think the plan is to make xpt files go away completely in bug 1447337. I mean, not really. The XPT files will still exist, they'll just have a different format. (And maybe be called something else...)

> I agree that the current state is not great, though it isn't too likely > anybody will mess with it. I think I could make them private if I define > constexpr ctors and use those to do the initialization. Do you want me to do > that in here, or can I leave that to a followup? A follow-up would be reasonable. I've been using constexpr ctors in bug 1411469 and they are doable with some effort. > Good point. Too much functional programming on my brain. I'll name it > "iterator" I guess. Or "iter" is a common abbreviation. > Are you thinking of a comment at the top of the definition of each variable > that explains what each field is, or a comment on every line explaining the > value? The latter seems kind of bloated to me, but I could try using the > existing method that converts each data structure to a string. The latter -- imagine debugging these lists if something goes wrong. Maybe it's not possible in a reasonable way, but if a single name can be emitted for some or all entries, for example, that could be very useful.

(In reply to Nicholas Nethercote [:njn] from comment #53) > Another question: what's the effect on the "xpti-working-set" value in > about:memory? Does it get rid of that entirely? Is the memory reporter still > required? Good point. I talked about that in one of my million comments in this bug, but I'll add something to the commit message for part 5 that spells it all out. Without my patch, in a local opt build xpti-working-set is 860,368 bytes. With my patch, it is 213,200 bytes. Comment 9 has some details on what remains, and I filed bug 1444745 for eliminating about half of what remains. The memory reporting did not change because the memory I saved is all allocated using XPTArena, but until bug 1444745 is fixed that is still used.

(In reply to Nicholas Nethercote [:njn] from comment #56) > A follow-up would be reasonable. I've been using constexpr ctors in bug > 1411469 and they are doable with some effort. Filed bug 1448454. > Or "iter" is a common abbreviation. iter is a built in function in Python, so I didn't want to call it that. > The latter -- imagine debugging these lists if something goes wrong. Maybe > it's not possible in a reasonable way, but if a single name can be emitted > for some or all entries, for example, that could be very useful. I actually did spend some time debugging these lists, so I don't have to imagine. ;) If you are trying to look up the definition of a particular interface, say nsIAccessibleScrollType, you first go down to the sStrings section and search for the name of the interface. That will give you a comment like "// nsIAccessibleScrollType 8823". 8823 is the offset for this string. Then you go up to sInterfaces and search for " 8823," The space and comma are needed to avoid confusing it with, say, 18823. That takes you right to this entry: {{0x05cd38b1, 0x94b3, 0x4cdf, {0x83, 0x71, 0x39, 0x35, 0xa9, 0x61, 0x14, 0x05}}, 8823, 301, 240, 36, 0, 7, 0xa0}. If you are trying to figure out which interface an entry is for, you look at xpt.py to see which field is the name offset, then go to the sStrings part of the file and look for 8823. It is a little clunky, but not too bad. I'll add the name of each interface. That should be easy and won't take up too much space.

(In reply to Mike Hommey [:glandium] from comment #51) > Gah, so you still generate xpt files... you'll want to make them never end > up in dist/bin, which I don't think you're doing here. Yeah, the basic idea is that to avoid the speed problems of bug 1267337 which are caused by needing to reparse all XPIDL files any time any XPIDL file is changed, we need some kind of intermediate representation. XPT files work just fine, and have the build system infrastructure already set up to make them, including the various tricky dependency cases, so I left them as is to try to make the patch simpler. I could put them in another directory, though. (I was doing that at some point.) Maybe dist/idl? Or I could create a dist/xpt directory? I don't know anything about the directory organization of the objdir.

(In reply to Nicholas Nethercote [:njn] from comment #43) > That's unfortunate, because it could cause problems for people bisecting. > Are you planning to roll these patches up before landing? My own bias is as a person who spends much more time looking through old diffs than bisecting, so I prefer the patches split up. People break the tree enough that I don't feel like adding in a few more broken builds hurts that much. If you prefer, I could comment out kHeader so the intermediate steps wouldn't build at all, to fail fast for any unlucky person who ends on on one of the intermediate patches.

(In reply to Andrew McCreight [:mccr8] from comment #59) > I could put them in another directory, though. (I was doing that at some > point.) Maybe dist/idl? Or I could create a dist/xpt directory? I don't know > anything about the directory organization of the objdir. Just in the local objdir ($objdir/config/makefiles/xpidl) would be fine. Which means, from the corresponding makefile, they just don't need to be prefixed with a path.

(In reply to Mike Hommey [:glandium] from comment #61) > Just in the local objdir ($objdir/config/makefiles/xpidl) would be fine. > Which means, from the corresponding makefile, they just don't need to be > prefixed with a path. Ah, that makes a lot of sense, thanks. It looks like this means I can also remove the substitution of @xpt_files@ from recursivemake.py, because xpt_files is now just "add .xpt to the end of every xpt_module".

Comment on attachment 8957576 [details] Bug 1438688, part 3 - Remove XPT files from the packaging process. https://reviewboard.mozilla.org/r/226458/#review236818 I still think the packaging code (mozpack) should be left alone. ::: config/makefiles/xpidl/Makefile.in:52 (Diff revision 4) > - $(call py_action,buildlist,$@ $(foreach xpt,$(filter $*/%,$(registered_xpt_files)),'interfaces $(notdir $(xpt))')) > - > -interfaces_manifests := @interfaces_manifests@ > - > xpidl_modules := @xpidl_modules@ > -registered_xpt_files := @registered_xpt_files@ > +xpt_files := $(foreach module,$(xpidl_modules),$(module).xpt) $(addsuffix .xpt,$(xpidl_modules)) would do the same thing.

Comment on attachment 8959383 [details] Bug 1438688, part 6 - Compile XPT information to C++ at build time. https://reviewboard.mozilla.org/r/228210/#review236840 Is there a reason to keep XPTHeader as an actual struct? XPTHeader::kHeader.mFoo could be XPTHeader::kFoo, and that would avoid intermediate pointers and the corresponding relocations. ::: config/makefiles/xpidl/Makefile.in:33 (Diff revision 3) > > 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 generating a file in a different directory, and using that file there, is not great. The build system won't know it has to build the cpp from here before doing stuff in xpcom/typelib/xpt. You'll probably want input from chmanchester or mshal for this, as this also has an impact on the tup build backend. ::: xpcom/typelib/xpt/tools/xpt.py:191 (Diff revision 3) > + # Store each string as individual characters to work around > + # MSVC's limit of 65k characters for a single string literal > + # (error C1091). And I was going to comment that it would be nicer if it were a big string :(

(In reply to Mike Hommey [:glandium] from comment #70) > Is there a reason to keep XPTHeader as an actual struct? > XPTHeader::kHeader.mFoo could be XPTHeader::kFoo, and that would avoid > intermediate pointers and the corresponding relocations. Oh, good point! There's probably not. I'll look into that. > generating a file in a different directory, and using that file there, is > not great. The build system won't know it has to build the cpp from here > before doing stuff in xpcom/typelib/xpt. > > You'll probably want input from chmanchester or mshal for this, as this also > has an impact on the tup build backend. In one iteration, I was generating it in a make file for the directory, but there was some reason that not having it in the same make file as the XPT files was causing an issue. I can set that up again and try to recall what the problem was, in case there's something that can be fixed. > And I was going to comment that it would be nicer if it were a big string :( Yeah, it used to be like that, until I realized it broke the build on Windows...

(In reply to Mike Hommey [:glandium] from comment #70) > You'll probably want input from chmanchester or mshal for this, as this also > has an impact on the tup build backend. I emailed them and they said it would be straightforward for them to fix this in tup once my patch lands. Is that sufficient to address your concern on this point? (Separately, I've updated the patches to remove XPTHeader, and I reverted the changes to mozpack locally.)

I'll just throw my updated patches here for review and see what you think. Nick, part 2b is some minor changes needed to get rid of XPTHeader.

Comment on attachment 8963463 [details] Bug 1438688, part 2b - Eliminate XPTHeader data structure. https://reviewboard.mozilla.org/r/232394/#review237796 ::: xpcom/typelib/xpt/xpt_struct.h:29 (Diff revision 1) > struct XPTTypeDescriptor; > struct XPTTypeDescriptorPrefix; > > struct XPTHeader { > - uint16_t mNumInterfaces; > - const XPTInterfaceDirectoryEntry* mInterfaceDirectory; > + static const uint16_t kNumInterfaces; > + static const XPTInterfaceDirectoryEntry kInterfaceDirectory[]; I can't work out where these fields are defined.

(In reply to Nicholas Nethercote [:njn] from comment #81) > I can't work out where these fields are defined. They get defined in the C++ file generated by the Python script in the new version of part 6.

I'll just clear the redundant ni.

Comment on attachment 8957576 [details] Bug 1438688, part 3 - Remove XPT files from the packaging process. https://reviewboard.mozilla.org/r/226458/#review238590

Comment on attachment 8959383 [details] Bug 1438688, part 6 - Compile XPT information to C++ at build time. https://reviewboard.mozilla.org/r/228210/#review238594

Thanks for all of the reviews.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/08b1a5f904e4 part 1 - Add methods for accessing arrays in xpt_struct.h. r=njn https://hg.mozilla.org/integration/autoland/rev/186f916dcc7a part 2 - Load XPT information from a static variable instead of a file. r=njn https://hg.mozilla.org/integration/autoland/rev/4da0e1839353 part 2b - Eliminate XPTHeader data structure. r=njn https://hg.mozilla.org/integration/autoland/rev/2f243bca1af3 part 3 - Remove XPT files from the packaging process. r=glandium https://hg.mozilla.org/integration/autoland/rev/4c437ba9d984 part 4 - Hoist arrays to XPTHeader. r=njn https://hg.mozilla.org/integration/autoland/rev/e05ec1e08b46 part 5 - Merge XPTInterfaceDirectoryEntry and XPTInterfaceDescriptor. r=njn https://hg.mozilla.org/integration/autoland/rev/8786eabb61a4 part 6 - Compile XPT information to C++ at build time. r=glandium,njn

Backed out 7 changesets (bug 1438688) for android xpcshell failures on builds/worker/workspace/build/tests/bin/components/test_necko.xpt Log of the failure: https://treeherder.mozilla.org/logviewer.html#?job_id=171526058&repo=autoland&lineNumber=1505 Backout: https://hg.mozilla.org/integration/autoland/rev/70400746eb530da029289760b9bab836558f9824 Push that failed: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8786eabb61a41e1b83b69bf8f1f30d135184c404

It looks like I missed an xpt reference. The Android xpcshell harness explicitly copies the file bin/components/test_necko.xpt. That worked fine, albeit uselessly, when I did my Android try push, but I suspect that when I updated my patch to stop dumping the xpt files in bin/components this broke. Presumably just not copying the file will work, but I'll do a try push to make sure.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4f8ea03dbe4d part 1 - Add methods for accessing arrays in xpt_struct.h. r=njn https://hg.mozilla.org/integration/autoland/rev/e3b0f068f61e part 2 - Load XPT information from a static variable instead of a file. r=njn https://hg.mozilla.org/integration/autoland/rev/1f3b60346b77 part 2b - Eliminate XPTHeader data structure. r=njn https://hg.mozilla.org/integration/autoland/rev/af6c0673ec6f part 3 - Remove XPT files from the packaging process. r=glandium https://hg.mozilla.org/integration/autoland/rev/fe00ed08356d part 4 - Hoist arrays to XPTHeader. r=njn https://hg.mozilla.org/integration/autoland/rev/f1dd107a1ad4 part 5 - Merge XPTInterfaceDirectoryEntry and XPTInterfaceDescriptor. r=njn https://hg.mozilla.org/integration/autoland/rev/102872860c7a part 6 - Compile XPT information to C++ at build time. r=glandium,njn

https://hg.mozilla.org/mozilla-central/rev/4f8ea03dbe4d https://hg.mozilla.org/mozilla-central/rev/e3b0f068f61e https://hg.mozilla.org/mozilla-central/rev/1f3b60346b77 https://hg.mozilla.org/mozilla-central/rev/af6c0673ec6f https://hg.mozilla.org/mozilla-central/rev/fe00ed08356d https://hg.mozilla.org/mozilla-central/rev/f1dd107a1ad4 https://hg.mozilla.org/mozilla-central/rev/102872860c7a

It looks like this regressed libxul size by 335k, although it's possible follow ups improved things or it shifted runtime overhead to static overhead. https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=mozilla-inbound,1299711,0,2&series=autoland,1338582,1,2&zoom=1522728998892.7212,1522729655332.4463,129620332.94217125,129977597.45493963&selected=autoland,1338582,322437,441932742,2

Hmm ok. I checked locally that it improved the installer size, but maybe I didn't check libxul itself.

I guess the XPT data was originally around 300kb, so it would make sense that it would make the executable that much larger. I'm still storing the same data (though I did do a few things to reduce how much there is.)