nsNetUtil.h implements all of its functions inline, which is why it needs to #include lots of other headers. This slows down the compilation of all the files that include nsNetUtils.h, of which there are about 500 at the moment. To me it seems like the implementation of the nsNetUtils functions could be split into an nsNetUtils.cpp file. Are there reasons against doing this?

Go for it. The reason for keeping it .h used to be that we couldn't link non-XPCOM code across modules. We haven't enforced that for a long time now IIRC, so we should now be able to keep all the logic in a .cpp file.

This is how far I've got. I don't really have any idea what I'm doing, and netwerk/test/ fails to link, so I'd like to pass this on to somebody who knows something about linkage.

Can you attach the output of the linker errors? It may be something simple. > #ifdef MOZILLA_INTERNAL_API Getting rid of the #if..else here might be the issue. But that's just a wild guess.

Sure: > 0:51.08 Undefined symbols for architecture x86_64: > 0:51.08 "NS_NewURI(nsIURI**, char const*, nsIURI*, nsIIOService*)", referenced from: > 0:51.08 TestSuccess(char const*, bool, unsigned long long, bool, nsISiteSecurityService*, nsIPermissionManager*) in TestSTSParser.o > 0:51.08 TestFailure(char const*, nsISiteSecurityService*, nsIPermissionManager*) in TestSTSParser.o > 0:51.08 ld: symbol(s) not found for architecture x86_64

Seems like we should have a networking person working on this, not Markus. I think Markus would agree, hope nobody minds me doing this. Jason - can you find a new owner?

Volunteers accepted for this bug! (I don't think it actually takes necko expertise--at this point it's mainly a matter of figuring out some linker errors). I'll find someone if no one steps up--this could be a nice build-time win.

I'd like to take this. I had some ideas about how to do the split so I basically started from scratch, but looking at Markus' patch I think we're doing the same thing. If I get too lost in the forest of missing includes (resulting from moving the ones in nsNetUtil.h to nsNetUtil.cpp) I'll pick up Markus' patch and work on the linker errors, but I think I'm doing okay for now.

This seemed like a good volunteer bug so I've started to work on it. I started from scratch since I wanted to do it in individual steps as small (and review-able) as possible. Step 1 was to move copy all of nsNetUtil.h to a new nsNetUtil.cpp, add nsNetUtil.cpp to moz.build and get this to compile by: - removing function implementations from the .h - removing default parameter values from the .cpp - removing templates from the .cpp Step 1 should work on its own. Afterward, I plan to go through nsNetUtil.h and start removing headers it includes one by one in separate patches. Currently step 1 doesn't work on its own because various test executables won't link. For example: 1:21.91 Executing: c++ -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-var-tracking -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -gsplit-dwarf -fno-omit-frame-pointer -o TestIncrementalDownload /home/catalin/hacking/moz/mozilla-central/obj-x86_64-unknown-linux-gnu/netwerk/test/tmpVUYBNw.list -lpthread -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -B ../../build/unix/gold -Wl,-rpath-link,/home/catalin/hacking/moz/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin -Wl,-rpath-link,/usr/local/lib -L../../dist/bin -L../../dist/lib ../../dist/bin/libxul.so ../../dist/bin/libmozalloc.so -L/home/catalin/hacking/moz/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/lib -lnspr4 -lplc4 -lplds4 ../../dist/lib/libxpcomglue_s.a ../../dist/bin/libxul.so ../../dist/bin/libmozalloc.so -L/home/catalin/hacking/moz/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/lib -lnspr4 -lplc4 -lplds4 ../../dist/lib/libjs_static.a -Wl,--whole-archive ../../dist/lib/libmozglue.a ../../dist/lib/libmemory.a -Wl,--no-whole-archive -rdynamic -ldl 1:21.91 /home/catalin/hacking/moz/mozilla-central/obj-x86_64-unknown-linux-gnu/netwerk/test/tmpVUYBNw.list: 1:21.91 INPUT("TestIncrementalDownload.o") 1:21.91 INPUT("../../modules/zlib/src/adler32.o") 1:21.92 INPUT("../../modules/zlib/src/compress.o") 1:21.92 INPUT("../../modules/zlib/src/crc32.o") 1:21.92 INPUT("../../modules/zlib/src/deflate.o") 1:21.92 INPUT("../../modules/zlib/src/gzclose.o") 1:21.92 INPUT("../../modules/zlib/src/gzlib.o") 1:21.92 INPUT("../../modules/zlib/src/gzread.o") 1:21.92 INPUT("../../modules/zlib/src/gzwrite.o") 1:21.92 INPUT("../../modules/zlib/src/infback.o") 1:21.92 INPUT("../../modules/zlib/src/inffast.o") 1:21.92 INPUT("../../modules/zlib/src/inflate.o") 1:21.92 INPUT("../../modules/zlib/src/inftrees.o") 1:21.92 INPUT("../../modules/zlib/src/trees.o") 1:21.92 INPUT("../../modules/zlib/src/uncompr.o") 1:21.92 INPUT("../../modules/zlib/src/zutil.o") 1:21.92 1:21.92 /home/catalin/hacking/moz/mozilla-central/netwerk/test/TestIncrementalDownload.cpp:80: error: undefined reference to 'NS_NewURI(nsIURI**, char const*, nsIURI*, nsIIOService*)' 1:21.92 collect2: error: ld returned 1 exit status 1:21.92 gmake[6]: *** [TestIncrementalDownload] Error 1 It seems to me like NS_NewURI(nsIURI**, char const*, nsIURI*, nsIIOService*) should be found in libxul.so which is included in the link command that fails catalin@opensuse:~/hacking/moz/mozilla-central> nm -C obj-x86_64-unknown-linux-gnu/dist/bin/libxul.so | grep NS_NewURI 00000000030b8445 t NS_NewURIFixup(nsIURIFixup**) 0000000000bef7be t NS_NewURI(nsIURI**, char const*, nsIURI*, nsIIOService*) 0000000000bef740 t NS_NewURI(nsIURI**, nsAString_internal const&, char const*, nsIURI*, nsIIOService*) 0000000000bef6b9 t NS_NewURI(nsIURI**, nsACString_internal const&, char const*, nsIURI*, nsIIOService*) Jason, do you see some obvious thing I'm missing? Should I do more to the build system than adding 'nsNetUtil.cpp' to SOURCES in netwerk/base/src/moz.build?

Apologies for my silence here - I ran into the same problem, then got bogged down with classes. I noticed (and worked around) one other problem: nsNetUtil.h includes various defines that change the way functions are compiled. It also includes some template functions which could be instantiated in various ways. I worked around this by splitting these functions off into nsNetUtil-inl.h (which includes nsNetUtil.h), allowing me to further limit the amount of headers included in nsNetUtil.h.

> /TestIncrementalDownload.cpp:80: error: undefined reference to > 'NS_NewURI(nsIURI**, char const*, nsIURI*, nsIIOService*)' > > nm -C obj-x86_64-unknown-linux-gnu/dist/bin/libxul.so | grep NS_NewURI > 0000000000bef7be t NS_NewURI(nsIURI**, char const*, nsIURI*, nsIIOService*) That really is strange--I can't think of why the linker is not finding the function in libxul. cc-ing bsmedberg, on the wild guess that he knows linker stuff like this better. emanuel, what do you mean exactly by "nsNetUtil.h includes various defines that change the way functions are compiled"?

Sorry, that was poorly phrased. I meant there are sections of code that do something different depending on what is #defined at compilation time. So depending on what's being compiled - for instance, whether it's MOZILLA_INTERNAL_API or not - the compiler will generate different code. Moving these sections to a .cpp file breaks these compile-time choices since only one version of the code will be compiled, so some functions need to remain inline. Since these functions still pull in some dependencies, and not that many files actually depend on them, I moved them into a nsNetUtil-inl.h file - but maybe calling it nsNetUtil2.h would be better, since this isn't like the SpiderMonkey idiom: these are functions, not classes, so there's no reason to declare them in nsNetUtil.h as well (and risk running into the used-but-never-defined trap). It also might not be worth it to move them out at all, since it doesn't save that many includes, but I was already done sprinkling those all around the codebase anyway when I realized the problem :) It's unfortunate that Catalin and I both seem to have complete patch sets here already (although neither has been posted yet) - how should we proceed here once the linker problem is taken care of? I'm somewhat hoping that the above gives my version an edge in terms of completeness, but it's not really that hard to correct.

Ah yes, the MOZILLA_INTERNAL_API issue is probably what's causing this. > It's unfortunate that Catalin and I both seem to have complete patch sets here already Indeed. Thanks to both of you for working on it. It sounds like at this point your (Emanuel) patches might be closer to being ready, but I don't know how much each of you are available to work on this, so can I ask the two of you to sort it out? Whoever takes it, just assign yourself to the bug (and post a patch :)

Also worth looking at how big firefox exe is with and without patch. I suspect that for some of the shorter, more commonly used functions, it may still be a win to inline them (especially if they don't require lots of #includes we wouldn't need otherwise). Most obvious are the versions of functions (NS_NewURI, etc) that just call the same function with a different signature after some minor parameter tweaking (NS_ConvertUTF16toUTF8, etc). But if there's not much of an exe size delta, then probably don't waste much time on that.

I still run into a similar linker error: mozilla-central/netwerk/test/TestBlockingSocket.cpp:52: error: undefined reference to 'NS_NewLocalFileInputStream(nsIInputStream**, nsIFile*, int, int, int)' This function is declared in nsNetUtil.h and (in the new setup) defined in nsNetUtil.cpp - I can't see any defines that would mess things up here. I could just try moving it but since I don't see a reason for the error, I'm worried that it's just the tip of the iceberg.

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #11) > It's unfortunate that Catalin and I both seem to have complete patch sets > here already (although neither has been posted yet) - how should we proceed > here once the linker problem is taken care of? My patches are far from complete, I've only just started, found the linker issue and wanted to ask here before moving forward. So I haven't actually removed any includes from nsNetUtil.h. I should have asked if you're still working on it, but on the other hand at least I revived the discussion and confirmed your linker issue (which I also don't think has anything to do with MOZILLA_INTERNAL_API). I have no problem with your patches going in since you seem a lot closer to being done.

Great! I'm doing some cleanup as we wait for Benjamin's input, and I'll assign myself to prevent further confusion.

If I recall correctly, the patch I attached in comment 2 already has a minimal set of #includes and compiles, it just does not link. I started the other way round: I removed all includes first and then added them as necessary.

What input are you looking for exactly? If you implement a function in a .cpp file, it's not going to be exported from libxul and therefore won't be available to external-linkage things like tests. That's why netutil is mostly inline. Or you can make the tests not use any netutils functions.

That makes an awful lot of sense. So the question is: do we want these functions to have external visibility in general? For the tests, would it be possible to define inline shim functions, or would those run into the same problem? (I'm not really sure where internal linkage ends and external linkage begins)

Emanuel: can you figure out how many of these functions are being used by tests? If it's a small number, we can work around this by 1) duplicating the code in the tests, or 2) having a separate 'nsNetUtil-tests.h' file that inlines the needed functions (and is used only by tests, or 3) maybe some other kludge where we have a 3rd file that gets included either in nsNetUtil.h or in .cpp depending on whether INTERNAL_API is defined. At least those are the first ideas that come to mind to fix this.

(In reply to Jason Duell (:jduell) from comment #20) > Emanuel: can you figure out how many of these functions are being used by > tests? If it's a small number, we can work around this by 1) duplicating > the code in the tests, or 2) having a separate 'nsNetUtil-tests.h' file that > inlines the needed functions (and is used only by tests, or 3) maybe some > other kludge where we have a 3rd file that gets included either in > nsNetUtil.h or in .cpp depending on whether INTERNAL_API is defined. At > least those are the first ideas that come to mind to fix this. Please not option #1 nor option #2. I suggest we define all the functions in nsNetUtil.inl and then have nsNetUtil.cpp and nsNetUtil-tests.h #include "nsNetUtil.inl". Please no copy/pasting.

I basically agree with Brian--I'd only go for #1 or #2 if the number of function involved was small and the body was fairly trivial.

Status update: I got this to compile and link on Linux opt and debug. A recent change to nsNetUtil.h bitrotted me somewhat, but caused me to rethink some things. It seems Gonk-only defines, at least, will be defined for everything in the build, so there's no need make functions depending on those inline. I'm going to check if the same applies to Android. Further, it occurs to me that anything that doesn't have MOZILLA_INTERNAL_API will by definition not be able to see anything in nsNetUtil.cpp, so I should be able to unify nsNetUtil-tests.h and nsNetUtil-inl.h for that case, and include this file in nsNetUtil.h when MOZILLA_INTERNAL_API isn't defined. That should make working with this a lot nicer for future users. I expect to have this ready tomorrow.

This just moves nsNetUtil.h to nsNetUtil.cpp in netwerk/base/src to make the next part a little easier to read.

This patch does the actual splitting, and makes the required moz.build changes. A few things of note: 1) With this patch, the only inline functions that nsNetUtil.h contains are the variants of NS_QueryNotificationCallbacks. Most of these needed to be inline for template specialization, and it seemed unreasonable to split them up. Creating a separate file for them also seemed overkill as they only depend on two additional headers. 2) The functions that need to be seen by external code or that change their behavior when used by external code are contained in nsNetUtil.inl. This file is included at the end of nsNetUtil.h, but only when the code including nsNetUtil.h does not have MOZILLA_INTERNAL_API defined. This is a little unorthodox, but allows files to simply include nsNetUtil.h without having to worry about whether they're internal or external. 3) When the functions in nsNetUtil.inl are included by external code, they need to be marked inline to avoid multiple definitions. When they are included by nsNetUtil.cpp, they cannot be inline since other modules have to see them. Therefore I created the 'INLINE_IF_EXTERN' macro that expands to 'MOZ_NEVER_INLINE' for internal code and 'MOZ_ALWAYS_INLINE' for external code. Simply using or omitting the 'inline' keyword also works, but I thought it best to be explicit about the intention. 4) nsNetUtil.cpp includes a few functions that change their behavior or are omitted entirely depending on the presence of certain defines. I confirmed locally that the build system takes care of this (MOZ_WIDGET_GONK is only defined when compiling Gonk, and MOZ_WIDGET_ANDROID is only defined when compiling for Android), so these functions do not need to be inline.

And finally, this patch adds all the includes needed after minimizing the set in nsNetUtil.h and nsNetUtil.inl. This affects a lot of files, but keep in mind that these headers (and many more) were already being implicitly included by including nsNetUtil.h before. Some files even implicitly include nsNetUtil.h! Parts 0, 1 and 2 need to be applied together to get a working build. With these patches applied to the parent revision (b9980eb88a26) I got a green run on try for all tier 1 platforms, but part 2 will bitrot fairly quickly. I'm a little worried about nonstandard build configurations, as they may be using files not touched in the configurations I tested, but unless there are specific ones you think I should test I don't think I can do much better than what tryserver gives me access to :)

(In reply to Jason Duell (:jduell) from comment #13) > Also worth looking at how big firefox exe is with and without patch. I didn't see any significant difference in size for either firefox.exe or xul.dll on a Windows build. I also ran the build through Talos on a few platforms and didn't see any significant changes (a few outliers, but they went away with retriggers as far as I can tell).

Bonus patch! I decided to audit the places where nsNetUtil.h is included across the tree. This removes all the places that included nsNetUtil.h without using any of its functions or defines, and adds a few places that were implicitly including it. To do this I went through each function in nsNetUtil.h and enumerated its uses across the tree. So another thing I can do is annotate what each include is for (since I have that data now), which might be helpful for future readers.

Comment on attachment 8337180 [details] [diff] [review] Part 1: Split nsNetUtil.h into nsNetUtil.h, nsNetUtil.cpp and nsNetUtil.inl Oh, something I forgot to mention about this patch: since I was touching them all anyway, I decided to make the function header style consistent. I chose foo(bar* baz) since it seemed at least slightly prevailing, but if foo(bar *baz) is preferred here I can change it.

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #29) > Comment on attachment 8337180 [details] [diff] [review] > Part 1: Split nsNetUtil.h into nsNetUtil.h, nsNetUtil.cpp and nsNetUtil.inl > > Oh, something I forgot to mention about this patch: since I was touching > them all anyway, I decided to make the function header style consistent. I > chose foo(bar* baz) since it seemed at least slightly prevailing, but if > foo(bar *baz) is preferred here I can change it. That's correct according to the style guide.

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #28) > So another thing I can do is annotate what each > include is for (since I have that data now), which might be helpful for > future readers. This patch implements this for all 318 (after part 3) includes of nsNetUtil.h in the tree. It's a big patch, but it's all just comment changes.

Jason: Gentle review ping

Comment on attachment 8337175 [details] [diff] [review] Part 0: Move nsNetUtil.h to make the next part read a little easier Emanuel tells me he's going to uplift new version of these patches (they're heavily bitrotted because I took too long to review them). This is a good bug to fix though--it could significantly reduce the number of files needed during incremental recompilations--so I'll make sure we move on it quickly when we have new patches.

Comment on attachment 8608075 [details] [diff] [review] fix missing includes Review of attachment 8608075 [details] [diff] [review]: ----------------------------------------------------------------- Wow--this must have been a fun patch to write. How many times did you run the compiler to figure all this out? Yikes :)

I imagine these patches could bitrot quickly. You've got my +r for anything bitrot modifications as long as the patches pass try. Let's get this landed finally ASAP.

(In reply to Dragana Damjanovic [:dragana] from comment #39) > Created attachment 8629974 [details] [diff] [review] > part1_bug_905127_v1.patch this failed to apply: patching file dom/media/webaudio/AudioContext.cpp Hunk #1 FAILED at 29 1 out of 1 hunks FAILED -- saving rejects to file dom/media/webaudio/AudioContext.cpp.rej patching file media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp Hunk #1 FAILED at 64 1 out of 1 hunks FAILED -- saving rejects to file media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp.rej could you take a look, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/40d08326c27f https://hg.mozilla.org/integration/mozilla-inbound/rev/91c69276782a https://hg.mozilla.org/integration/mozilla-inbound/rev/cf774875a1f4

https://hg.mozilla.org/mozilla-central/rev/40d08326c27f https://hg.mozilla.org/mozilla-central/rev/91c69276782a https://hg.mozilla.org/mozilla-central/rev/cf774875a1f4

The changes that were made for this bug, are causing some linker errors: https://bugzilla.mozilla.org/show_bug.cgi?id=1246338 would someone be able to take a look.