1347679 - Deduce ContextProfile and version via queries

So the patch here introduced the first ever <regex> usage in our codebase, which needs some extra care for the potential bloat of codesize and time to complie. I just asked the implementor of libstdc++'s <regex>, and he told me there is no extern explicit instantiation for <regex> in at least libstdc++, which means if we use it in our codebase, all the codesize of <regex> would go into libxul. And IIRC, regex engines usually have a relative large codesize which we want to avoid embedding prematurely.

Xidorn Quan [:xidorn] UTC+10

Comment 8

•

8 years ago

And the implementor told me that according to his measurement, <regex> takes 260KB ~ 291KB codesize, which sounds like a normal size for a regex engine. froydnj, I recall your words in bug 1328497 comment 16, what do you think about this? Also, if we have to introduce a separate regex engine anyway (which eshan also wants in bug 1310335 comment 41), should we use the Rust regex crate rather than <regex> given they would both bloat our own binary? I guess we don't want both, so we should either go the Rust way or the C++ stdlib.

Flags: needinfo?(nfroyd)

Nathan Froyd [:froydnj]

Comment 9

•

8 years ago

Thanks for pointing this out. A couple thoughts: 1) 250K or so is a lot of codesize for what (at this point) is a relatively minor patch. On that basis alone, I'm inclined to ask for the patch to be backed out. This definitely shows up in our Android installer size tracking, though it's not immediately obvious, since that tracking is so noisy. :( 1a) I'd be really curious to see what this looks like on Windows, since libstdc++ and libc++ are basically header-only implementations. 2) Before we start using <regex>, we need to review it for exception safety and include it in config/stl-headers. <regex> should not have been used without that review. (This is non-obvious, though. :( ) This is also grounds for a backout. 3) Having one regex implementation that we call from C++ would be ideal; I am inclined to say that we should use the Rust one, but that requires C++ bindings (which are not written yet), it sounds like the regex engines support slightly different syntaxes, and that will undoubtedly trip up people who Just Want To Use <regex>. But maybe these are not a problem, since we already have our own strings, hashtables, etc. 4) Third-party code using <regex> may also complicate this story somewhat, since we can easily pull in third-party code that uses <regex> and be stuck with the codesize increase regardless. Jeff, can you please back this out for points 1) and 2) above? Thanks.

Flags: needinfo?(nfroyd) → needinfo?(jgilbert)

Nika Layzell [:nika] (ni? for response)

Comment 10

•

8 years ago

(In reply to Nathan Froyd [:froydnj] from comment #9) > 4) Third-party code using <regex> may also complicate this story somewhat, > since we can easily pull in third-party code that uses <regex> and be stuck > with the codesize increase regardless. If we decide to use <regex> it will be much the same issue the other way around, in that we could easily accidentally pull in regex (the crate) when pulling in rust dependencies. As far as I can tell it looks like it is going to be more common for us to pull in new rust dependencies than C++ ones, but I can't know for sure.

Nika Layzell [:nika] (ni? for response)

Comment 11

•

8 years ago

(In reply to Michael Layzell [:mystor] from comment #10) > (In reply to Nathan Froyd [:froydnj] from comment #9) > > 4) Third-party code using <regex> may also complicate this story somewhat, > > since we can easily pull in third-party code that uses <regex> and be stuck > > with the codesize increase regardless. > > If we decide to use <regex> it will be much the same issue the other way > around, in that we could easily accidentally pull in regex (the crate) when > pulling in rust dependencies. As far as I can tell it looks like it is going > to be more common for us to pull in new rust dependencies than C++ ones, but > I can't know for sure. I feel like I should note, however, that we could probably use cargo replace rules to rewrite all dependencies on the rust regex crate to a shim crate which proxies the requests over IPC to the C++ <regex> header, but that might be really, really slow.

Kelsey Gilbert [:jgilbert]

Assignee

Comment 12

•

8 years ago

This patch adds 23KB to opt-build xul.dll. I'm double-checking what it looks like on linux clang.

Nathan Froyd [:froydnj]

Comment 13

•

8 years ago

(In reply to Jeff Gilbert [:jgilbert] from comment #12) > This patch adds 23KB to opt-build xul.dll. Wow, that's much less than I would have expected...I guess dead function removal must have been effective in removing a lot of things. But still!

Xidorn Quan [:xidorn] UTC+10

Comment 14

•

8 years ago

From prefherder, it seems this change only adds ~15KB to Android installer size: https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800&series=%5Bmozilla-inbound,7601f68563dc6dad10bc630a38bb80e7b78fe5b1,1,2%5D&series=%5Bautoland,7601f68563dc6dad10bc630a38bb80e7b78fe5b1,1,2%5D&highlightedRevisions=1da903d4071b Maybe not a serious issue then. (In reply to Nathan Froyd [:froydnj] from comment #13) > Wow, that's much less than I would have expected...I guess dead function > removal must have been effective in removing a lot of things. But still! I don't think there are too many dead function there... unless the compiler analyzes the regex string in compile time...

Kelsey Gilbert [:jgilbert]

Assignee

Comment 15

•

8 years ago

Unpatched: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40452ba991abfdc3afceca3fc67bfdf02dded6ef Patched: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b03b6fa32fd6a07e85c62c3e989244337789bc53 Linux x64 pgo: Size of libxul.so: 100879712->100987480 (+108KB) Size of target.tar.bz2: 67091415->67146806 (+55KB) Windows XP opt: Size of firefox-55.0a1.en-US.win32.zip: 54553280->54560197 (+6917 bytes) Windows 8 x64 opt: Size of firefox-55.0a1.en-US.win64.zip: 59603081->59621244 (+18KB) Android 4.0 API15+ opt: Size of libxul.so: 21200900->21207796 (+6896 bytes) Size of target.apk: 34888213->34895107 (+6894 bytes) Mac build had to be retriggered, so still waiting on that.

Kelsey Gilbert [:jgilbert]

Assignee

Comment 16

•

8 years ago

There is one previous usage of <regex> in the tree: security/nss/nss-tool/db/dbtool.cc:13:#include <regex> Not sure if nss-tool is built into xul.

Kelsey Gilbert [:jgilbert]

Assignee

Updated

•

8 years ago

Depends on: 1349064

Nika Layzell [:nika] (ni? for response)

Comment 17

•

8 years ago

If we do decide to not use <regex> in tree, would it be useful to have a static analysis which complains if one of these forbidden headers is included? I'm not sure if there are any other headers which we're avoiding due to code size concerns.

Kelsey Gilbert [:jgilbert]

Assignee

Comment 18

•

8 years ago

(In reply to Nathan Froyd [:froydnj] from comment #9) > 2) Before we start using <regex>, we need to review it for exception safety > and include it in config/stl-headers. <regex> should not have been used > without that review. (This is non-obvious, though. :( ) This is also > grounds for a backout. This is now bug 1349064. > Jeff, can you please back this out for points 1) and 2) above? Thanks. I'll have it backed out if you have any concerns in bug 1349064. Otherwise, I'd prefer to minimize churn here, particularly since this has gone to central now.

Flags: needinfo?(jgilbert) → needinfo?(nfroyd)

Nathan Froyd [:froydnj]

Comment 19

•

8 years ago

Thanks for doing size measurements in comment 15. (In reply to Jeff Gilbert [:jgilbert] from comment #18) > > Jeff, can you please back this out for points 1) and 2) above? Thanks. > > I'll have it backed out if you have any concerns in bug 1349064. Otherwise, > I'd prefer to minimize churn here, particularly since this has gone to > central now. I would like to minimize churn here too. Do you think you'll be able to address the comments in bug 1349064 in short order?

Flags: needinfo?(nfroyd)

Nathan Froyd [:froydnj]

Comment 20

•

8 years ago

(In reply to Michael Layzell [:mystor] from comment #17) > If we do decide to not use <regex> in tree, would it be useful to have a > static analysis which complains if one of these forbidden headers is > included? I'm not sure if there are any other headers which we're avoiding > due to code size concerns. This is bug 1349700.