Closed Bug 1411450 Opened 7 years ago Closed 7 years ago

Remove static constructors from protobuf

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla58

Tracking Flags:

Tracking

Status

firefox-esr52

---

unaffected

firefox56

---

unaffected

firefox57

---

unaffected

firefox58

---

fixed

People

(Reporter: francois, Assigned: francois)

References

Details

(Whiteboard: pwphish-prep)

Attachments

(3 files)

Bug 1411450 - Switch devtools CoreDump.proto to the LITE_RUNTIME. 7 years ago François Marier [:francois] 59 bytes, text/x-review-board-request	fitzgen : review+	Details
Bug 1411450 - Add the protobuf version to LayerScopePacket.proto. 7 years ago François Marier [:francois] 59 bytes, text/x-review-board-request	mattwoodrow : review+	Details
Bug 1411450 - Only build files required for the "lite" protobuf runtime. 7 years ago François Marier [:francois] 59 bytes, text/x-review-board-request	mossop : review+	Details

François Marier [:francois]

Assignee

Description

•

7 years ago

Bug 1385461 introduced 27 new static constructors: https://bugzilla.mozilla.org/show_bug.cgi?id=1385461#c58

François Marier [:francois]

Assignee

Updated

•

7 years ago

status-firefox56: --- → unaffected

status-firefox57: --- → unaffected

status-firefox58: --- → affected

status-firefox-esr52: --- → unaffected

François Marier [:francois]

Assignee

Comment 1

•

7 years ago

Ionuț, is there a way to run the test you linked to (https://treeherder.mozilla.org/perf.html#/alerts?id=10139) locally or on Try?

I'd like to see if this Chromium patch makes a difference: https://cs.chromium.org/chromium/src/third_party/protobuf/patches/0003-remove-static-initializers.patch?sq=package:chromium

Flags: needinfo?(igoldan)

Ionuț Goldan [:igoldan]

Comment 2

•

7 years ago

You can push to Try the 2 changesets you want to compare. Then use the compare tool from
https://treeherder.mozilla.org/perf.html#/comparechooser
to actually compare the results of test runs.
Make sure that you select "build_metrics" from the left side dropdown menu. That's the category for the compare_metrics alerts.

Flags: needinfo?(igoldan)

Comment hidden (mozreview-request)

The "lite" runtime allows us to pull in a lot less of the protocol
buffers library:

https://developers.google.com/protocol-buffers/docs/proto#options

Review commit: https://reviewboard.mozilla.org/r/195238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195238/

Comment hidden (mozreview-request)

This is essentially a no-op but it does silence the following warning
whenever the *.pb.* files are re-generated:

[libprotobuf WARNING google/protobuf/compiler/parser.cc:546] No syntax specified for the proto file: LayerScopePacket.proto. Please use 'syntax = "proto2";' or 'syntax = "proto3";' to specify a syntax version. (Defaulted to proto2 syntax.)

Review commit: https://reviewboard.mozilla.org/r/195240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195240/

Comment hidden (mozreview-request)

All of our .proto are optimized for LITE_RUNTIME so we can use
libprotobuf-lite instead of the full libprotobuf.

Review commit: https://reviewboard.mozilla.org/r/195242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195242/

François Marier [:francois]

Assignee

Comment 6

•

7 years ago

Based on an earlier try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93f12f338e63810159632810c987c113ad123d66

perfherder: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=93f12f338e63&framework=2&filter=static_constructors&selectedTimeRange=172800

shows that the number of static constructors went down compared to bug 1385461:

			orig	1385461	now
linux32	opt		103	130	117
linux32 debug		213	241	227
linux64	no-opt debug	223	251	237
linux32 pgo		103	130	117
linux64 pgo		103	130	117
linux32-rusttests opt	103	130	117
linux32-rusttests debug	213	241	227
linux64-rusttests opt	103	130	117
linux64-rusttests debug	213	241	227

The Chromium patch mentioned in comment 1 doesn't change anything for us: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=caa66ec1736ef0839e642f4d9127a913c1412091&framework=2&filter=static_constructors&selectedTimeRange=172800

François Marier [:francois]

Assignee

Comment 7

•

7 years ago

Nick: the changes to .pb.h and .pb.cc are mechanical and simply the result of selecting the lite runtime.

Code Review Bot [:reviewbot]

Comment 8

•

7 years ago

mozreview-review

Comment on attachment 8924021 [details]
Bug 1411450 - Switch devtools CoreDump.proto to the LITE_RUNTIME.

https://reviewboard.mozilla.org/r/195238/#review200264


C/C++ static analysis found 5 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/shared/heapsnapshot/CoreDump.pb.cc:125
(Diff revision 1)
>  #if !defined(_MSC_VER) || _MSC_VER >= 1900
>  const int Metadata::kTimeStampFieldNumber;
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  Metadata::Metadata()
> -  : ::google::protobuf::Message(), _internal_metadata_(NULL) {
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {
                                                           ^
                                                           nullptr

::: devtools/shared/heapsnapshot/CoreDump.pb.cc:360
(Diff revision 1)
>  const int StackFrame_Data::kIsSystemFieldNumber;
>  const int StackFrame_Data::kIsSelfHostedFieldNumber;
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  StackFrame_Data::StackFrame_Data()
> -  : ::google::protobuf::Message(), _internal_metadata_(NULL) {
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {
                                                           ^
                                                           nullptr

::: devtools/shared/heapsnapshot/CoreDump.pb.cc:1382
(Diff revision 1)
>  const int StackFrame::kDataFieldNumber;
>  const int StackFrame::kRefFieldNumber;
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  StackFrame::StackFrame()
> -  : ::google::protobuf::Message(), _internal_metadata_(NULL) {
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {
                                                           ^
                                                           nullptr

::: devtools/shared/heapsnapshot/CoreDump.pb.cc:1755
(Diff revision 1)
>  const int Node::kScriptFilenameFieldNumber;
>  const int Node::kScriptFilenameRefFieldNumber;
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  Node::Node()
> -  : ::google::protobuf::Message(), _internal_metadata_(NULL) {
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {
                                                           ^
                                                           nullptr

::: devtools/shared/heapsnapshot/CoreDump.pb.cc:2979
(Diff revision 1)
>  const int Edge::kNameFieldNumber;
>  const int Edge::kNameRefFieldNumber;
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  Edge::Edge()
> -  : ::google::protobuf::Message(), _internal_metadata_(NULL) {
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {
                                                           ^
                                                           nullptr

François Marier [:francois]

Assignee

Comment 9

•

7 years ago

(In reply to Static Analysis Bot [:clangbot] from comment #8)
> If you see a problem in this automated review, please report it here:
> http://bit.ly/2y9N9Vx

Filed https://github.com/mozilla-releng/services/issues/699

Whiteboard: pwphish-prep

Matt Woodrow (:mattwoodrow)

Comment 10

•

7 years ago

mozreview-review

Comment on attachment 8924022 [details]
Bug 1411450 - Add the protobuf version to LayerScopePacket.proto.

https://reviewboard.mozilla.org/r/195240/#review200328

Attachment #8924022 - Flags: review?(matt.woodrow) → review+

Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]

Comment 11

•

7 years ago

The reason we weren't using the LITE_RUNTIME was because the gzip streams were not available with it -- has that changed? Do the devtools/shared/heapsnapshot/tests/** tests still run and pass OK?

If so, r=me; if not, then we can't use the LITE_RUNTIME here.

François Marier [:francois]

Assignee

Comment 12

•

7 years ago

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #11)
> The reason we weren't using the LITE_RUNTIME was because the gzip streams
> were not available with it -- has that changed?

You're right, libprotobuf-lite doesn't include it: https://github.com/google/protobuf/blob/cbe250591fca9d2e776776be065a72c5550a5556/src/Makefile.am#L191-L232

but I did keep it in our builds: https://hg.mozilla.org/users/fmarier_mozilla.com/mozilla-unified/rev/e8092a5702835ac841d46eca2faca3ea7f1a569c#l2.65

So I guess I should be more accurate and say that we're linking against "libprotobuf-lite+gzip_stream".

> Do the devtools/shared/heapsnapshot/tests/** tests still run and pass OK?

I ran the following tests and they were all successful:

./mach test devtools/shared/heapsnapshot/tests/
./mach test devtools/shared/heapsnapshot/tests/unit
./mach test devtools/shared/heapsnapshot/tests/mochitest/test_DominatorTree_01.html
./mach test devtools/shared/heapsnapshot/tests/mochitest/test_saveHeapSnapshot_e10s_01.html
./mach test devtools/shared/heapsnapshot/tests/mochitest/test_SaveHeapSnapshot.html

I'm not sure how to run the gtests manually but as far as I can see they were part of this Try run and all passed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca655db16ea59977c4c66b57603f15ad7e3062d6

Is there another test I should run to confirm that this devtools feature still works?

Flags: needinfo?(nfitzgerald)

Comment hidden (mozreview-request)

Comment on attachment 8924023 [details]
Bug 1411450 - Only build files required for the "lite" protobuf runtime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195242/diff/1-2/

Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]

Comment 14

•

7 years ago

That looks good to me -- thanks for checking!

Flags: needinfo?(nfitzgerald)

Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]

Comment 15

•

7 years ago

mozreview-review

Comment on attachment 8924021 [details]
Bug 1411450 - Switch devtools CoreDump.proto to the LITE_RUNTIME.

https://reviewboard.mozilla.org/r/195238/#review200632

Attachment #8924021 - Flags: review?(nfitzgerald) → review+

François Marier [:francois]

Assignee

Comment 16

•

7 years ago

I added a note in the README documenting the devtools' dependency on gzip streams: https://reviewboard.mozilla.org/r/195242/diff/1-2/

Code Review Bot [:reviewbot]

Comment 17

•

7 years ago

mozreview-review

Comment on attachment 8924021 [details]
Bug 1411450 - Switch devtools CoreDump.proto to the LITE_RUNTIME.

https://reviewboard.mozilla.org/r/195238/#review200636


C/C++ static analysis found 5 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/shared/heapsnapshot/CoreDump.pb.cc:125
(Diff revision 1)
>  #if !defined(_MSC_VER) || _MSC_VER >= 1900
>  const int Metadata::kTimeStampFieldNumber;
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  Metadata::Metadata()
> -  : ::google::protobuf::Message(), _internal_metadata_(NULL) {
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {
                                                           ^
                                                           nullptr

::: devtools/shared/heapsnapshot/CoreDump.pb.cc:360
(Diff revision 1)
>  const int StackFrame_Data::kIsSystemFieldNumber;
>  const int StackFrame_Data::kIsSelfHostedFieldNumber;
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  StackFrame_Data::StackFrame_Data()
> -  : ::google::protobuf::Message(), _internal_metadata_(NULL) {
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {
                                                           ^
                                                           nullptr

::: devtools/shared/heapsnapshot/CoreDump.pb.cc:1382
(Diff revision 1)
>  const int StackFrame::kDataFieldNumber;
>  const int StackFrame::kRefFieldNumber;
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  StackFrame::StackFrame()
> -  : ::google::protobuf::Message(), _internal_metadata_(NULL) {
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {
                                                           ^
                                                           nullptr

::: devtools/shared/heapsnapshot/CoreDump.pb.cc:1755
(Diff revision 1)
>  const int Node::kScriptFilenameFieldNumber;
>  const int Node::kScriptFilenameRefFieldNumber;
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  Node::Node()
> -  : ::google::protobuf::Message(), _internal_metadata_(NULL) {
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {
                                                           ^
                                                           nullptr

::: devtools/shared/heapsnapshot/CoreDump.pb.cc:2979
(Diff revision 1)
>  const int Edge::kNameFieldNumber;
>  const int Edge::kNameRefFieldNumber;
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  Edge::Edge()
> -  : ::google::protobuf::Message(), _internal_metadata_(NULL) {
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) {
                                                           ^
                                                           nullptr

François Marier [:francois]

Assignee

Updated

•

7 years ago

Attachment #8924023 - Flags: review?(mh+mozilla) → review?(dtownsend)

François Marier [:francois]

Assignee

Comment 18

•

7 years ago

Sorry for the noise Mike.

This change isn't really build-system related, it's simply trimming the protobuf files we include down to just what we need. Moving the review to Dave in his capacity as Toolkit module owner.

Dave Townsend [:mossop]

Comment 19

•

7 years ago

mozreview-review

Comment on attachment 8924023 [details]
Bug 1411450 - Only build files required for the "lite" protobuf runtime.

https://reviewboard.mozilla.org/r/195242/#review201150

Attachment #8924023 - Flags: review?(dtownsend) → review+

Pulsebot

Comment 20

•

7 years ago

Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1dfd453ab88e
Switch devtools CoreDump.proto to the LITE_RUNTIME. r=fitzgen
https://hg.mozilla.org/integration/autoland/rev/aceeaac5c23f
Add the protobuf version to LayerScopePacket.proto. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/387253168deb
Only build files required for the "lite" protobuf runtime. r=mossop

Narcis Beleuzu [:NarcisB]

Comment 21

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/1dfd453ab88e
https://hg.mozilla.org/mozilla-central/rev/aceeaac5c23f
https://hg.mozilla.org/mozilla-central/rev/387253168deb

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox58: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla58

Sylvestre Ledru [:Sylvestre]

Comment 22

•

7 years ago

Positive side effect with coverity:
    Analysis Summary:
       New defects found: 2 (not here)
       Defects eliminated: 120

You need to log in before you can comment on or make changes to this bug.