Closed Bug 1347679 Opened 3 years ago Closed 3 years ago

Deduce ContextProfile and version via queries

Categories

(Core :: Graphics, enhancement)

Product:
Core
Component:
Graphics
Type:
enhancement
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(1 file)

Bug 1347679 - Determine ContextProfile from driver and simplify version parsing. -
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details 
Currently we explicitly specify which profile a GLContext has at context creation time.
We can instead ask GL and determine it.
Comment on attachment 8847757 [details]
Bug 1347679 - Determine ContextProfile from driver and simplify version parsing. -

https://reviewboard.mozilla.org/r/120680/#review123318

LGTM
Attachment #8847757 - Flags: review?(dmu) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da903d4071b
Determine ContextProfile from driver and simplify version parsing. - r=daoshengmu
https://hg.mozilla.org/mozilla-central/rev/1da903d4071b
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
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)
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)
(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.
(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.
This patch adds 23KB to opt-build xul.dll.
I'm double-checking what it looks like on linux clang.
(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!
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...
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.
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.
Depends on: 1349064
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.
(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)
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)
(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.
You need to log in before you can comment on or make changes to this bug.